Skip to content

[FLINK-40382][build] Stop surefire from hiding nested test classes - #28962

Open
spuru9 wants to merge 1 commit into
apache:masterfrom
spuru9:FLINK-nested-test-fix
Open

[FLINK-40382][build] Stop surefire from hiding nested test classes#28962
spuru9 wants to merge 1 commit into
apache:masterfrom
spuru9:FLINK-nested-test-fix

Conversation

@spuru9

@spuru9 spuru9 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Reworked after review feedback — this now fixes the root cause in pom.xml instead of restructuring test code. The previous revision moved 15 nested classes around in three files (+595 lines); that fixed those three files and left the mechanism in place. Thanks @snuyanzin.

What is the purpose of the change

A test class declared as a static nested class is never selected:

  • Surefire's default excludes drop **/*$*, so nested class files are never scanned.
  • JUnit Jupiter only auto-discovers @Nested inner classes.

A static nested class falls through both. And because the enclosing class is often abstract, surefire emits nothing for it either — no Running line, no report file, not even Tests run: 0. Nothing in a build indicates the omission.

Brief change log

Override <excludes> on the default-test execution, which displaces surefire's defaults (specifying any <excludes> replaces the default list wholesale).

The explicit **/*$* exclude on the integration-tests execution is deliberately left alone. Its include is **/*.*, which would otherwise match Scala-generated names such as TypeInformationGenTest$$anon$85$$anon$86 — the case the existing comment there warns about. 500 Scala test files remain in flink-table-api-scala, -scala-bridge and the planner. The default-test include is name-based (**/*Test.*, **/*Tests.*), which those names do not match, so relaxing it is safe.

Verifying this change

No test code is touched. Locally, per module:

Module Before After
flink-table-type-utils 151 418
flink-core — both NullableSerializerTest variants absent 19 each
flink-runtimeInputSelectionTest$BuilderTest absent 3

Known problem with this revision. CI shows that lifting the exclude makes already-correct @Nested classes run twice: surefire selects both the enclosing class and the nested one, and JUnit can only deduplicate when both land in the same fork. In the table job, 141 classes report two summary lines (127 of them nested), i.e. roughly +518 redundant test executions in the planner alone. An earlier single-fork experiment suggested deduplication happened; that was wrong. This needs resolving before the change is mergeable.

Of the 152 nested classes that newly match the include across already-built modules, 134 are existing @Nested classes that already ran, 2 are abstract bases that JUnit ignores, and the remainder are the genuinely dead ones.

Also verified: with **/*.* and no $ exclude, anonymous classes (Foo$1), helper nested classes and test-less *Test-named helpers are silently ignored rather than erroring.

Note when testing locally: mvn clean is required. Stale target/test-classes from a branch that restructured these classes produces phantom IllegalAccessErrors.

flink-table-planner could not be verified locally — it holds 134 of the 152 newly-matched classes and all the Scala test sources, so CI is the real check for this change.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no (restores coverage of RowDataSerializer, ExternalSerializer, TimestampDataSerializer, NullableSerializer)
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no

AI assistance

  • This contribution was created with the assistance of an AI tool: Claude Code

@flinkbot

flinkbot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@spuru9
spuru9 force-pushed the FLINK-nested-test-fix branch 2 times, most recently from b990445 to f9996cd Compare August 13, 2026 06:22
@spuru9 spuru9 changed the title [FLINK-XXXXX][tests] Execute TimestampDataSerializerTest variants [FLINK-40382][tests] Execute the serializer test variants in flink-table-type-utils Aug 13, 2026
@spuru9

spuru9 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

A lot of whitespaces so might want to use hide whitespace setting while reviewing
image

@spuru9

spuru9 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Tests count increase for flink-table-type-utils — 151 → 418 in CI

BEFORE — green master build, pre-fix:
https://github.com/apache/flink/actions/runs/31416084933/job/93547589740

AFTER — PR #28962, commit f9996cd:
https://github.com/spuru9/flink/actions/runs/31673577371/job/94364624989

Both are the Test (module: misc) job — that's where flink-table-type-utils runs,

@spuru9
spuru9 marked this pull request as ready for review August 13, 2026 10:09
@spuru9

spuru9 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@snuyanzin @rmetzger Can you take a look.

@snuyanzin snuyanzin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking here

this approach changes lots of code however it does not fix the root cause
instead only moves it under the rug...

Surefire excludes **/$

why can't we overwrite this exclude for everyone?

@spuru9

spuru9 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

why can't we overwrite this exclude for everyone?

That would be a better way. Checking if there is any blocker in the same.

Surefire's default excludes drop **/*$*, so a test class declared as a
static nested class is never selected. JUnit Jupiter only auto-discovers
@nested inner classes, so such a class falls through both paths and never
runs, with no log line, report file or skip count to show it.

Override the excludes on the default-test execution. The include pattern
there is name-based (**/*Test.*, **/*Tests.*), so only nested classes whose
name ends in Test/Tests are picked up. The explicit **/*$* exclude on the
integration-tests execution is left alone: its include is **/*.*, which
would otherwise match Scala-generated names such as
TypeInformationGenTest$$anon$85$$anon$86.

This recovers the nested test classes without touching any test code:
flink-table-type-utils goes from 151 to 418 tests, flink-core picks up both
NullableSerializerTest variants (19 each) and flink-runtime picks up
InputSelectionTest$BuilderTest (3).

Of the 152 nested classes that newly match the include, 134 are existing
@nested classes that already ran (JUnit deduplicates, so they do not run
twice) and 2 are abstract bases that JUnit ignores.

Generated-by: Claude Code (claude-opus-5)
@spuru9
spuru9 force-pushed the FLINK-nested-test-fix branch from f9996cd to 3120939 Compare August 13, 2026 18:23
@spuru9 spuru9 changed the title [FLINK-40382][tests] Execute the serializer test variants in flink-table-type-utils [FLINK-40382][build] Stop surefire from hiding nested test classes Aug 13, 2026
@spuru9
spuru9 marked this pull request as draft August 13, 2026 19:53
@spuru9

spuru9 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@snuyanzin
I've pushed that instead: pom.xml, no test code touched, flink-table-type-utils goes 151 → 418, CI green. I only relaxed default-test; the **/*$* on integration-tests still guards the Scala-generated $$anon$ names.

There is a issue: surefire then selects both the enclosing and the nested class, and JUnit only deduplicates within a fork — so 127 already-correct @Nested classes run twice (~3.3s, 0.07% of the table job) i.e. 506 duplicate executions / 3.3s (0.07%).


AFTER (pom change applied)

https://github.com/spuru9/flink/actions/runs/31730469547/job/94552235499 — Test (module: table)

18:41:35.826 [INFO] Running …aggfunctions.MaxWithRetractAggFunctionTest$ByteMaxWithRetractAggFunctionTest
18:41:35.850 [INFO] Tests run: 4, Failures: 0, … Time elapsed: 0.015 s -- in …$ByteMaxWithRetractAggFunctionTest
18:41:36.114 [INFO] Running …aggfunctions.MaxWithRetractAggFunctionTest$ByteMaxWithRetractAggFunctionTest
18:41:36.127 [INFO] Tests run: 4, Failures: 0, … Time elapsed: 0.010 s -- in …$ByteMaxWithRetractAggFunctionTest

BEFORE (green master, same job)

https://github.com/apache/flink/actions/runs/31416084933/job/93547589715 — Test (module: table)

18:07:33.852 [INFO] Running …$ByteMaxWithRetractAggFunctionTest
18:07:33.864 [INFO] Tests run: 4, … -- in …$ByteMaxWithRetractAggFunctionTest

@spuru9
spuru9 marked this pull request as ready for review August 13, 2026 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants