[
https://issues.apache.org/jira/browse/GROOVY-12145?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King reassigned GROOVY-12145:
----------------------------------
Assignee: Paul King
> JUnit 5 migration silently disabled ~195 test methods: base class removed but
> @Test not added
> ---------------------------------------------------------------------------------------------
>
> Key: GROOVY-12145
> URL: https://issues.apache.org/jira/browse/GROOVY-12145
> Project: Groovy
> Issue Type: Bug
> Reporter: Leonard Brünings
> Assignee: Paul King
> Priority: Major
>
> h2. Summary
> The JUnit 5 migration commit {{a1897e0811}} ("Convert to JUnit5", Daniel Sun)
> converted many JUnit3-style Groovy tests ({{extends GroovyTestCase}}, methods
> named {{void testXxx()}}) to JUnit 5. The mechanical part of the conversion
> was to (a) drop the {{GroovyTestCase}} base class and (b) add {{@Test}} to
> every {{void testXxx()}} method.
> For a number of files step (b) was *partially or completely skipped*. Because
> the build uses {{useJUnitPlatform()}} and the JUnit Platform discovers tests
> *only* by the {{@Test}} annotation (there is no longer a {{TestCase}} base
> class for the vintage engine to discover), every un-annotated {{void
> testXxx()}} method *silently stopped executing*. The build stays green
> because "0 tests" is not a failure.
> *Headline numbers* (see the Totals table for the full breakdown, and
> "Verification and confidence" for how each portion was established):
> * *~195 dead test methods across 32 files* total, spanning {{a1897e0811}} and
> its sibling JUnit 5 migration commits.
> * *~175 of those (27 files) are attributable to {{a1897e0811}} alone*; the
> remaining ~20 (5 files) come from sibling migration commits and are listed
> under category (c) for completeness.
> * *~169 methods across 19 classes are fully dead and were confirmed by
> execution* (the class discovers *0 tests*). The remaining *~26 partially-dead
> methods are source-inferred only* (method present, annotation absent) and
> were *not* individually confirmed by a per-method execution diff.
> This is the same defect already known for {{groovy-console
> AstNodeToScriptAdapterTest}} (63 methods) — see "Already tracked" below. That
> file is *not* included in the counts here because it is being fixed under a
> separate change.
> h2. Root cause
> * {{extends GroovyTestCase}} was removed from the class.
> * {{@Test}} was *not* added (or was added to only some methods).
> * No {{@RunWith}} / JUnit4 style and no {{Specification}} base remain.
> * Result: JUnit Platform discovers *nothing* (fully-dead file) or *only the
> annotated subset* (partially-dead file). The remaining {{testXxx()}} methods
> never run.
> A secondary variant of the same commit: in several bug tests the injected
> {{import org.junit.jupiter.api.Test}} was placed *inside a triple-quoted
> script string* (e.g. inside {{shell.evaluate """ ... """}}) instead of at the
> top of the file — so the import is dead code inside test data and no
> annotation was ever added.
> A third, *collateral* variant: {{a1897e0811}} removed {{extends
> GroovyTestCase}} from the *abstract base* {{BuilderTestSupport}}. Its two
> concrete subclasses were never edited, but they inherited neither {{@Test}}
> nor a discovering base, so they died too.
> h2. Reproduction
> Any listed fully-dead class produces "No tests found", e.g. (verified
> empirically):
> {code}
> ./gradlew :groovy-astbuilder:test --tests
> 'org.codehaus.groovy.ast.builder.AstBuilderFromSpecificationTest'
> > Task :groovy-astbuilder:test FAILED
> Gradle Test Run :groovy-astbuilder:test: 0 passed, 0 skipped (0 tests)
> > No tests found for given includes:
> [org.codehaus.groovy.ast.builder.AstBuilderFromSpecificationTest](--tests
> filter)
> {code}
> For a file that still {{extends groovy.test.GroovyTestCase}} the vintage
> engine runs it normally — those files are *fine* and are excluded from this
> report.
> h2. Verification and confidence
> *This section is authoritative.* Where the per-file tables below (which were
> derived statically from source) disagree with what is stated here, the
> executed/verified facts here win.
> h3. Fully-dead classes — verified by execution
> Every *fully-dead* class was confirmed by running it through a {{--tests}}
> filter and observing {{0 tests}} / "No tests found" — not merely inferred
> from source. Use the *fully-qualified* class name; a {{*Wildcard}} pattern
> does not filter reliably in this build and silently runs a broad set instead.
> *Confirmed dead (0 tests discovered):*
> * Core {{:test}} (13):
> {{org.codehaus.groovy.runtime.trampoline.TrampolineTest}},
> {{bugs.Groovy779_Bug}}, {{groovy.util.ResourceBundleTest}},
> {{org.codehaus.groovy.tools.rootloadersync.SubclassingInGroovyTest}},
> {{org.codehaus.groovy.runtime.memoize.CacheCleanupTest}},
> {{bugs.Groovy4414Bug}}, {{groovy.SingletonBugTest}},
> {{groovy.StackTraceTest}}, {{bugs.Groovy7031Bug}}, {{bugs.Groovy8212Bug}},
> {{bugs.Groovy4080Bug}}, {{bugs.Groovy6755Bug}}, {{bugs.Groovy8138Bug}}
> * {{:groovy-astbuilder}} (1):
> {{org.codehaus.groovy.ast.builder.AstBuilderFromSpecificationTest}}
> * {{:groovy-xml}} (2, collateral via {{BuilderTestSupport}}):
> {{groovy.xml.MarkupBuilderTest}}, {{groovy.xml.StreamingMarkupBuilderTest}}
> * {{:groovy-templates}} (1): {{groovy.bugs.groovy4585.Groovy4585Bug}}
> * {{:groovy-json}} (1): {{json.JsonTest}}
> * {{:groovy-contracts}} (1):
> {{org.apache.groovy.contracts.tests.post.ImplicitVariableNamesPostconditionTests}}
> That is *19 classes verified by execution* (~169 dead test methods). These
> map onto the tables below as: category (a) minus its abstract base (15
> classes), category (a2) (2 classes), and the two *fully-dead* rows of
> category (c) — {{json.JsonTest}} and
> {{ImplicitVariableNamesPostconditionTests}}. The abstract base
> {{BuilderTestSupport}} cannot be run directly; its deadness is *inferred* but
> demonstrated in practice by its two concrete subclasses above, which do run
> and discover 0 tests.
> h3. Partially-dead classes — source-inferred only
> The per-file counts for the *partially-dead* tables ((b) and the partial rows
> of (c)) were derived *statically* and are *not* each confirmed by a
> per-method execution diff. Two caveats apply:
> * *A gross execution count cannot prove a specific named method is dead.* The
> partial-dead claims rest on source inspection (method present,
> {{@Test}}/{{@ParameterizedTest}} annotation absent) — sound, but not
> execution-confirmed per method.
> * *The static counts do not account for {{@ParameterizedTest}} methods*, so
> the number of executions a class reports can *exceed* its {{@Test}} count.
> Spot-checks: {{groovy.DynamicMemberTest}} ran 1 (matches its table),
> {{SortableTransformTest}} ran 11 (matches), but {{groovy.ForLoopTest}} ran
> *26* — parameterized inflation well above its tabulated 18 {{@Test}}. Its
> named victim {{testClassicForWithMultiAssignment2}} nonetheless carries no
> {{@Test}}/{{@ParameterizedTest}} and so is genuinely undiscoverable. Read the
> "# test methods" / "# @Test" columns below as *source counts*, not as
> execution counts.
> h2. (a) Fully-dead files caused by {{a1897e0811}}
> *Verified by execution* (each concrete class discovers 0 tests), except the
> abstract base {{BuilderTestSupport}}, whose deadness is inferred via its
> subclasses in (a2). Base class removed, *zero* {{@Test}} annotations added —
> every method below never runs.
> ||File||# test methods||# @Test||stray import-in-string||
> |{{subprojects/groovy-astbuilder/.../AstBuilderFromSpecificationTest.groovy}}|67|0|no|
> |{{subprojects/groovy-xml/.../groovy/xml/BuilderTestSupport.groovy}}
> (abstract base; inferred, see collateral below)|15|0|no|
> |{{src/test/groovy/org/codehaus/groovy/runtime/trampoline/TrampolineTest.groovy}}|4|0|no|
> |{{src/test/groovy/bugs/Groovy779_Bug.groovy}}|3|0|no|
> |{{src/test/groovy/groovy/util/ResourceBundleTest.groovy}}|3|0|no|
> |{{src/test/groovy/org/codehaus/groovy/tools/rootloadersync/SubclassingInGroovyTest.groovy}}|2|0|no|
> |{{src/test/groovy/org/codehaus/groovy/runtime/memoize/CacheCleanupTest.groovy}}|2|0|no|
> |{{src/test/groovy/bugs/Groovy4414Bug.groovy}}|2|0|no|
> |{{src/test/groovy/groovy/SingletonBugTest.groovy}}|2|0|no|
> |{{src/test/groovy/groovy/StackTraceTest.groovy}}|2|0|no|
> |{{subprojects/groovy-templates/.../groovy/bugs/groovy4585/Groovy4585Bug.groovy}}|1|0|no|
> |{{src/test/groovy/bugs/Groovy7031Bug.groovy}}|1|0|no|
> |{{src/test/groovy/bugs/Groovy8212Bug.groovy}}|1|0|no|
> |{{src/test/groovy/bugs/Groovy4080Bug.groovy}}|1|0|*YES*|
> |{{src/test/groovy/bugs/Groovy6755Bug.groovy}}|1|0|*YES*|
> |{{src/test/groovy/bugs/Groovy8138Bug.groovy}}|1|0|*YES*|
> *Subtotal: 16 files, 108 dead test methods* (3 with the stray
> import-in-string bug). Of these, 15 concrete classes are execution-confirmed;
> the abstract base {{BuilderTestSupport}} is inferred.
> h3. (a2) Collateral fully-dead files (base {{BuilderTestSupport}} was
> migrated)
> *Verified by execution.* These two files were *not* edited by {{a1897e0811}},
> but they {{extends BuilderTestSupport}}, whose {{GroovyTestCase}} base was
> removed. They have no {{@Test}} of their own, so all inherited + local tests
> are dead.
> ||File||# test methods (own + inherited)||# @Test||
> |{{subprojects/groovy-xml/.../groovy/xml/MarkupBuilderTest.groovy}}|29 (14
> own + 15 inherited)|0|
> |{{subprojects/groovy-xml/.../groovy/xml/StreamingMarkupBuilderTest.groovy}}|18
> (3 own + 15 inherited)|0|
> *Subtotal: 2 files, 47 dead test executions.*
> h2. (b) Partially-dead files caused by {{a1897e0811}}
> *Source-inferred only* (see "Partially-dead classes" above): the counts are
> static source counts and are not each confirmed by a per-method execution
> diff.
> h3. (b1) Base class removed, only *some* {{@Test}} added
> ||File||# test methods||# @Test||# dead||dead methods||
> |{{src/test/groovy/groovy/DynamicMemberTest.groovy}}|6|1|5|{{testGStringMethodCall}},
> {{testGStringPropertyAccess}}, {{testStringMethodCallAndAttributeAccess}},
> {{testDynamicAttributeAccess}}, {{testDynamicMethodClosure}}|
> |{{src/test/groovy/groovy/lang/ScriptCacheTest.groovy}}|2|1|1|{{testScriptCaching}}|
> h3. (b2) Touched by {{a1897e0811}}, single/few {{testXxx()}} left un-annotated
> The bulk of these classes are annotated, but the listed methods were
> overlooked. Verified as *standalone* tests (not helpers — each is referenced
> only by its own declaration).
> ||File||# test methods||# @Test||# dead||dead methods||
> |{{subprojects/groovy-json/.../groovy/json/JsonSlurperLaxTest.groovy}}|8 own
> (+60 inherited @Test)|60 inherited|8|{{testNullEmptyMalformedPayloads}},
> {{testObjectWithSimpleValues}}, {{testArrayOfArrayWithSimpleValues}},
> {{testBackSlashEscaping}}, {{testLaxCommentsAndKeys}},
> {{testInvalidNumbers}}*, {{testGroovy7728}}, {{testGroovy9954}}|
> |{{subprojects/groovy-json/.../groovy/json/JsonSlurperCharSourceTest.groovy}}|1
> own (+60 inherited @Test)|60 inherited|1|{{testParsingDecimalValue}}|
> |{{src/test/groovy/gls/annotations/AnnotationTest.groovy}}|64|63|1|{{testAnnotateAnnotationDefinitionWithAnnotationWithAnnotationTypeTarget}}|
> |{{src/test/groovy/org/codehaus/groovy/classgen/asm/sc/StaticCompileFlowTypingTest.groovy}}|7|6|1|{{testFlowTyping4}}|
> |{{src/test/groovy/org/codehaus/groovy/control/CompilerConfigurationTest.java}}|6|5|1|{{testSetViaSystemProperties}}|
> |{{src/test/groovy/bugs/Groovy11046.groovy}}|2|1|1|{{testMissingDependency2}}|
> |{{subprojects/groovy-contracts/.../tests/doc/RootClassExampleTests.groovy}}|18|17|1|{{test_postcond_with_result_and_old_variables_switched}}|
> {{*}} {{JsonSlurperLaxTest.testInvalidNumbers}} is an {{@Override}} of a
> {{@Test}} method in the parent {{JsonSlurperTest}}; whether it executes
> depends on Jupiter's inherited-annotation resolution — treat as suspect. The
> other 7 Lax-specific methods are unambiguously dead. ({{JsonSlurperLaxTest}}
> / {{JsonSlurperCharSourceTest}} still run the 60 inherited parent tests, so
> only the subclass-specific methods are lost.)
> *Subtotal (b): 9 files, 20 dead test methods (source-inferred).*
> h2. (c) Related — same defect, introduced by sibling JUnit 5 migration
> commits (not {{a1897e0811}})
> Found during the same sweep. Same root cause, different commit; listed for
> completeness. The two *fully dead* rows ({{JsonTest}},
> {{ImplicitVariableNamesPostconditionTests}}) are *execution-verified* (see
> above); the three partial rows are *source-inferred only* and are subject to
> the {{@ParameterizedTest}} caveat (notably {{ForLoopTest}} actually executes
> 26, above its tabulated 18 {{@Test}}).
> ||File||# test methods||# @Test||# dead||verification||migrating commit /
> cause||
> |{{subprojects/groovy-json/src/spec/test/json/JsonTest.groovy}}|13|0|13
> (fully dead)|execution-verified|{{92cbb3f39c}} "minor refactor: junit5" —
> base removed, no {{@Test}}|
> |{{src/test/groovy/org/codehaus/groovy/transform/SortableTransformTest.groovy}}|15|11|4|source-inferred|{{5afd9b9a91}}
> "junit jupiter" removed {{GroovyTestCase}} from base
> {{CompilableTestSupport}}|
> |{{src/test/groovy/gls/invocation/MethodSelectionTest.groovy}}|18|17|1|source-inferred|same
> — base {{CompilableTestSupport}}|
> |{{subprojects/groovy-contracts/.../tests/post/ImplicitVariableNamesPostconditionTests.groovy}}|1|0|1
> (fully dead)|execution-verified|{{extends BaseTestClass}} (no {{@Test}}, no
> discovering base); pre-existing / not {{a1897e0811}}|
> |{{src/test/groovy/groovy/ForLoopTest.groovy}}|20|18|1
> ({{testClassicForWithMultiAssignment2}})|source-inferred (class runs 26 via
> {{@ParameterizedTest}}; named victim still un-annotated)|migrated by an
> earlier commit; single missed {{@Test}}|
> *Subtotal (c): 5 files, 20 dead test methods* (2 files / 14 methods
> execution-verified fully dead; 3 files / 6 methods source-inferred partial).
> h2. Totals
> The "# Dead test methods" figures are the source-derived method counts; the
> "Verified by" column records how the deadness was established (execution vs.
> source inspection), per the "Verification and confidence" section.
> ||Category||Files||Dead test methods||Verified by||
> |(a) Fully-dead, {{a1897e0811}} base removed|16|108|execution (15 concrete
> classes) + inference for abstract base {{BuilderTestSupport}}|
> |(a2) Collateral fully-dead ({{BuilderTestSupport}}
> subclasses)|2|47|execution|
> |(b) Partially-dead, {{a1897e0811}}|9|20|source inspection only|
> |*{{a1897e0811}} total*|*27*|*~175*| |
> |(c) Related (sibling migration commits)|5|20|execution (2 fully-dead
> classes, 14 methods) + source inspection (3 partial classes, 6 methods)|
> |*Grand total*|*32*|*~195*| |
> *Verified-vs-inferred split of the grand total:* ~169 methods across *19
> fully-dead classes* are confirmed by execution; the remaining ~26 methods
> (categories (b) and the partial rows of (c)) are *source-inferred only*.
> h2. Suggested fix
> * For every file above, add {{@Test}} (import {{org.junit.jupiter.api.Test}})
> to each standalone {{void testXxx()}} method that should run.
> * For the *stray import-in-string* files ({{Groovy4080Bug}},
> {{Groovy6755Bug}}, {{Groovy8138Bug}}) — and the already-tracked
> {{AstNodeToScriptAdapterTest}} — move {{import org.junit.jupiter.api.Test}}
> out of the triple-quoted script string to the top-level import block, then
> annotate the methods.
> * For the {{BuilderTestSupport}} / {{CompilableTestSupport}} families: either
> annotate all inherited {{testXxx()}} in the concrete subclasses
> ({{MarkupBuilderTest}}, {{StreamingMarkupBuilderTest}},
> {{SortableTransformTest}}, {{MethodSelectionTest}}), or restore a
> JUnit-discoverable arrangement.
> * Consider a CI guard: fail the build (or a lint check) when a test *class
> file* named {{*Test}}/{{*Bug}} contributes *0* discovered tests, to prevent
> silent regressions of this kind.
> h2. Already tracked / excluded
> *
> {{subprojects/groovy-console/.../groovy/console/ui/AstNodeToScriptAdapterTest.groovy}}
> (63 dead methods + stray import-in-string in
> {{testAtImmutableClassWithProperties}}) is the original signature of this bug
> and is *already being fixed under a separate change* GROOVY-12144 —
> cross-reference only, not counted above.
> h3. Method notes (how the list was produced)
> * Started from the {{git show --name-only a1897e0811}} file set (1225 source
> files).
> * Parsed each file with string/comment masking so that {{void testXxx()}} and
> {{@Test}} occurrences *inside triple-quoted script strings* are ignored (this
> removed many false positives such as {{Groovy10133}} and
> {{TraitASTTransformationTest}}, whose "missing" annotations were embedded
> test scripts / a custom {{@CompileModesTest}} = {{@ParameterizedTest}}
> collector).
> * Recognized custom test meta-annotations (e.g. {{@ParameterizedTest}}
> collectors) and resolved class-inheritance chains so subclasses of a
> still-{{GroovyTestCase}} base (e.g. {{StaticImportTest extends
> groovy.test.GroovyTestCase}}) are correctly treated as *fine*.
> * Excluded parameterized/helper methods (those with a parameter list, e.g.
> {{AuthorTestHelper.testAuthorDataSet(Sql)}}) and private methods.
> * Confirmed attribution with {{git show a1897e0811 -- <file>}} (base-class
> removal + net {{@Test}} added).
> * *Fully-dead classes were then confirmed empirically* via Gradle
> ({{--tests}} filter → "No tests found"); *partially-dead classes were left at
> source-inferred confidence* because a gross execution count (inflated by
> {{@ParameterizedTest}}) cannot prove a specific named method is dead — see
> "Verification and confidence".
> {panel:title=Provenance}
> Assisted-by: Claude Code (Opus 4.8).
> {panel}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
