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 deterministic unique ids in Descriptions #1134

Merged
merged 2 commits into from
May 27, 2017

Conversation

mpkorstanje
Copy link
Contributor

@mpkorstanje mpkorstanje commented May 25, 2017

Summary

Improve surefire integration.

Motivation and Context

Rerunning failed tests with Surefire requires a method to identify the tests.
Surefire currently uses a tuple of (Class, Method) to identify failed tests.
Cucumber-jvm uses test suites which do not consist of Classes or methods.
Therefore another method is needed.

JUnit provides the option to select tests that match a Description.
Descriptions are compared using a unique Id. To identify tests between
different runs we should provide Descriptions with a predictable unique
id.

The current implementation did not suffice. While the provided cucumber
elements are unique, they are not predictable between runs making it
impossible to use their descriptions to rerun a failed test.

After this change it will be possible to update Surefire to use
descriptions to rerun failed tests.

Related Issues:

Closes #1120.

How Has This Been Tested?

Replaced ExecutionUnitRunnerTest#shouldPopulateRunnerStepsWithStepsUsedInStepDescriptions with FeatureRunnerTest#shouldPopulateDescriptionsWithStableUniqueIds that creates the same feature twice and tests the id's are unique and predictable.

The reproducer for Surefire can be found in my fork of Surefire. Running with cucumber 1.2.5 results in failed tests, with the result of this PR as 2.0.0-SNAPSHOT results in 2 flakes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Rerunning failed tests with Surefire requires a method to identify the tests.
Surefire currently uses a tuple of (Class, Method) to identify failed tests.
Cucumber-jvm uses test suites which do not consist of Classes or methods.
Therefore another method is needed.

JUnit provides the option to select tests that match a Description.
Descriptions are compared using an unique Id. To identify tests between
different runs we should provide Descriptions with a predictable unique
id.

The current implementation did not suffice. While the provided cucumber
elements are unique, they are not predictable. Each run would create a new
instance of the identifier making it impossible to use their descriptions
to rerun a failed test.

After this change it will be possible to update Surefire to use
descriptions to rerun failed tests.

  Related Issues:
  - https://issues.apache.org/jira/browse/SUREFIRE-1372
  - #1120
  - temyers/cucumber-jvm-parallel-plugin#31
@mpkorstanje mpkorstanje force-pushed the issue-1120-surefire-rerun branch 2 times, most recently from 2ca4488 to 3affec7 Compare May 26, 2017 19:53


static class WithStepDescriptions extends ParentRunner<PickleStep> implements PickleRunner {
private final Runner runner;
Copy link
Contributor Author

@mpkorstanje mpkorstanje May 26, 2017

Choose a reason for hiding this comment

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

Two separate classes because the semantics of ParentRunner somewhat expect there to be children. It can be hacked but doing so would make things fragile.

@@ -42,14 +42,14 @@ public void ensureOriginalDirectory() {
@Test
public void finds_features_based_on_implicit_package() throws IOException, InitializationError {
Cucumber cucumber = new Cucumber(ImplicitFeatureAndGluePath.class);
assertEquals(3, cucumber.getChildren().size());
assertEquals(2, cucumber.getChildren().size());
assertEquals("Feature: FA", cucumber.getChildren().get(0).getName());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I inlined feature_with_same_steps_in_different_scenarios.feature


PickleStepWrapper(PickleStep step) {
this.step = step;
PickleStepId(PickleEvent pickleEvent, PickleStep pickleStepLine) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it should be pickleStep instead of pickleStepLine, I fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct.

FeatureRunner rerunner = createFeatureRunner(cucumberFeature);

Set<Description> descriptions = new HashSet<Description>();
assertDescriptionIsUnique(runner.getDescription(), descriptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a bit unusual for an assert-method to modify its arguments, but checking the uniqueness with a set makes sence - and then the set has been create and can be used subsequently.



@Test
public void shouldPopulateDescriptionsWithStableUniqueIds() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Most test method uses the non-idomatic form of using underscore, since some/many of us contributors find them easier to read (and in tests readability is even more important than elsewhere), see discussion at #302 (comment)

@brasmusson brasmusson force-pushed the issue-1120-surefire-rerun branch from 3affec7 to 6bad552 Compare May 27, 2017 14:28
@brasmusson brasmusson merged commit 6bad552 into master May 27, 2017
brasmusson added a commit that referenced this pull request May 27, 2017
asfgit pushed a commit to apache/maven-surefire that referenced this pull request Jan 4, 2018
Description#createSuiteDescription

 Filtering tests by the tuple of (Class, Method) does not work when a test
 suite is used as a suite may not have (Class, Method) . Filtering by the
 description of the failed test should however work.

 Related Issues:
 - cucumber/cucumber-jvm#1134
 - temyers/cucumber-jvm-parallel-plugin#31

SUREFIRE-1372 Filter junit4 tests to be rerun by description
@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use surefire maven plugin to rerun failing tests
2 participants