Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build MethodSource with the correct spec class name. #1345

Merged
merged 7 commits into from
Jun 19, 2021

Conversation

milo2048
Copy link
Contributor

@milo2048 milo2048 commented Jun 10, 2021

The combination of Spock, Junit5, and the maven Surefire plugin prevents tests from running if they're inherited from a parent class.

Consider this scenario:

class MyParentSpec extends Specification {
    def 'some test'() {
        when: 'A thing happens'
        def foo = 'someValue'
        then: 'It works'
        assert foo == 'someValue'
   }
}

class MyChildSpec extends MyParentSpec() {
}

Run command line with:
mvn -Dtest=MyChildSpec

No tests will run. Ultimately this is because Spock reports a test plan to Junit5 wherein the test 'some test' belongs to MyParentSpec instead of MyChildSpec, and the Surefire plugin then filters out the test.

The same kind of inheritance scenario with a pure Junit5 test works as expected.

From the JUnit5 docs for MethodSource:

/**
	 * Create a new {@code MethodSource} using the supplied
	 * {@link Class class} and {@link Method method}.
	 *
	 * <p>This method should be used in favor of {@link #from(Method)} if the
	 * test method is inherited from a superclass or present as an interface
	 * {@code default} method.
	 *
	 * @param testClass the Java class; must not be {@code null}
	 * @param testMethod the Java method; must not be {@code null}
	 * @since 1.3
	 */
	@API(status = STABLE, since = "1.3")
	public static MethodSource from(Class<?> testClass, Method testMethod) {
		return new MethodSource(testClass, testMethod);
	}

Therefore I believe that Spock should be reporting 'some test' as belonging to MyChildSpec.

With the supplied patch, the tests run through maven/surefire.

@leonard84
Copy link
Member

Can you add tests for this, maybe by using the EngineTestKit ?

@milo2048 milo2048 force-pushed the fix-junit5-metadata branch 2 times, most recently from 5b13c23 to ffeedc5 Compare June 11, 2021 13:43
@milo2048
Copy link
Contributor Author

milo2048 commented Jun 11, 2021 via email

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #1345 (7cc0a98) into master (567b8b0) will increase coverage by 79.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1345       +/-   ##
=============================================
+ Coverage          0   79.20%   +79.20%     
- Complexity        0     3940     +3940     
=============================================
  Files             0      408      +408     
  Lines             0    12253    +12253     
  Branches          0     1616     +1616     
=============================================
+ Hits              0     9705     +9705     
- Misses            0     1967     +1967     
- Partials          0      581      +581     
Impacted Files Coverage Δ
...ain/java/org/spockframework/runtime/SpockNode.java 76.59% <100.00%> (ø)
.../org/spockframework/runtime/SpockTimeoutError.java 100.00% <0.00%> (ø)
.../org/spockframework/mock/EmptyOrDummyResponse.java 85.00% <0.00%> (ø)
...main/groovy/spock/util/EmbeddedSpecCompiler.groovy 94.59% <0.00%> (ø)
.../org/spockframework/builder/SetterSlotFactory.java 66.66% <0.00%> (ø)
...va/org/spockframework/runtime/model/ErrorInfo.java 100.00% <0.00%> (ø)
...rk/mock/constraint/WildcardArgumentConstraint.java 66.66% <0.00%> (ø)
...ock/util/SourceToAstNodeAndSourceTranspiler.groovy 76.87% <0.00%> (ø)
...java/org/spockframework/runtime/ValueRecorder.java 73.07% <0.00%> (ø)
...ava/org/spockframework/runtime/model/NodeInfo.java 100.00% <0.00%> (ø)
... and 399 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 567b8b0...7cc0a98. Read the comment docs.

@Vampire
Copy link
Member

Vampire commented Jun 18, 2021

This is a bug in the surefire plugin that has to be fixed there I think.
Gradle had the same issue and fixed it.
Quoting @marcphilipp who is both a JUnit and Gradle developer from #1235:

The MethodSource is supposed to contain the declaring class, hence "source", and was introduced to allow IDE navigation. The filtering code could check for the parent descriptor's ClassSource in this case instead of using the MethodSource's class name.

@Vampire
Copy link
Member

Vampire commented Jun 18, 2021

Here the PR that fixed it in Gradle: gradle/gradle#15943

@leonard84
Copy link
Member

Good point @Vampire, I completely forgot about that issue with gradle.

@leonard84
Copy link
Member

I talked with @marcphilipp again, and he corrected is statement from the other PR and mentioned this commit junit-team/junit5@8856b48

@leonard84 leonard84 requested a review from Vampire June 18, 2021 17:25
Copy link
Member

@leonard84 leonard84 left a comment

Choose a reason for hiding this comment

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

I've simplified the PR and removed the maven dependency, while preserving the effect of the maven test filter in the test.

@leonard84 leonard84 merged commit 6d2e6cc into spockframework:master Jun 19, 2021
@leonard84
Copy link
Member

Thanks @milo2048

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.

5 participants