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

Use Spring's @Rule support instead of @RunWith(SpringJunit4ClassRunner) #250

Merged
merged 1 commit into from
Nov 14, 2016

Conversation

catchin
Copy link
Contributor

@catchin catchin commented Nov 11, 2016

Use Spring's @rule support added in v4.2 for the SpringScenarioTest instead of using SpringJUnit4ClassRunner.

Enables the use of custom JUnit runners like DataProviderRunner (see example test).

Copy link
Contributor

@janschaefer janschaefer left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! It would be great if you could address the comments, mainly to keep the old rule-less way, so that users of old Spring versions are still supported.

@@ -19,10 +16,16 @@
*
* @since 0.8.0
*/
public class SpringScenarioTest<GIVEN, WHEN, THEN> extends
ScenarioTest<GIVEN, WHEN, THEN> implements BeanFactoryAware {
public class SpringScenarioTest<GIVEN, WHEN, THEN> extends InternalSpringScenarioTest<GIVEN, WHEN, THEN> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to introduce a new class here, something like RuleBasedSpringScenarioTest. Otherwise users of old Spring versions will not be able to use that class anymore.


import com.tngtech.jgiven.integration.spring.SpringScenarioTest;

@RunWith( SpringJUnit4ClassRunner.class )
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: we still need to support the runner-based execution

*
* @since 0.14.0
*/
public abstract class InternalSpringScenarioTest<GIVEN, WHEN, THEN> extends ScenarioTestBase<GIVEN, WHEN, THEN> implements BeanFactoryAware {
Copy link
Contributor

Choose a reason for hiding this comment

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

As the constructor is already package-private you could also make the class itself package-private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this does not work because of Springs ClassRule:
org.junit.internal.runners.rules.ValidationError: The @ClassRule 'springClassRule' must be declared in a public class.
Would you think it's more consistent to have the constructor public then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And move it to a package named "internal"?

Copy link
Contributor

Choose a reason for hiding this comment

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

no I think that would be overkill. The class is visible in any case, so just leave it as it is

public void setupSpring() {
wireSteps( new SpringCanWire( beanFactory ) );
}
@ContextConfiguration( classes = TestSpringConfig.class )
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, thanks for adapting that!

@catchin
Copy link
Contributor Author

catchin commented Nov 11, 2016

I created new classes as suggested.

@catchin
Copy link
Contributor Author

catchin commented Nov 11, 2016

Or SimpleSpringRuleScenarioTest and SpringRuleScenarioTest ?

@janschaefer
Copy link
Contributor

That is even better :-)

…arting with version 4.2, as an alternative to SpringScenarioTest using SpringJUnit4ClassRunner.

Enables the use of custom JUnit runners like DataProviderRunner.
@catchin
Copy link
Contributor Author

catchin commented Nov 12, 2016

Incorporated the changes.

@janschaefer janschaefer merged commit 6cf1dca into TNG:master Nov 14, 2016
@catchin
Copy link
Contributor Author

catchin commented Nov 14, 2016

fixes #229

@janschaefer janschaefer added this to the v0.13.0 milestone Nov 14, 2016
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.

2 participants