maven-surefire
maven-surefire copied to clipboard
apache
Adding support for externally passed random seed and printing used seed on console
It's good practice to execute tests in random order. That ensures tests are atomic and not depend on each other or some stateful entity. But, if such error occurs, right now, there is no way to reproduce this exact, erroneous execution.
This PR adds the ability to reproduce those errors by adding support for externally passed random seed. If random seed is not passed, it will be generated automatically. Also, random seed will be printed on console. That's enable it to be achieved by CI server for later.
This operation is strongly influenced by Ruby's rspec order command line option: https://www.relishapp.com/rspec/rspec-core/v/2-13/docs/command-line/order-new-in-rspec-core-2-8
Showcase of execution:
@cardil Would it be possible without adding a new config parameter random-seed?
Rosa Marozzi on [email protected] replies: Unregister from the list please thanks :) El 14/4/2016 10:19 p. m., "cardil" [email protected] escribi=C3=B3:
GitHub user cardil opened a pull request:
https://github.com/apache/maven-surefire/pull/112
Adding support for externally passed random seed and printing used seed
on console
It's good practice to execute tests in random order. That ensures tests
are atomic and not depend on each other or some stateful entity. But, if such error occurs, right now, there is no way to reproduce this exact, erroneous execution.
This PR adds the ability to reproduce those errors by adding support
for externally passed random seed. If random seed is not passed, it will be generated automatically. Also, random seed will be printed on console. That's enable it to be achieved by CI server for later.
This operation is strongly influenced by Ruby's rspec order command
line option: https://www.relishapp.com/rspec/rspec-core/v/2-13/docs/command-line/order-n= ew-in-rspec-core-2-8
Showcase of execution:
[](https://asciinema.org/a/42342)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wavesoftware/maven-surefire
feature/random-seed-usage
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/maven-surefire/pull/112.patch
To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message:
This closes #112
@Tibor17 Yes. Actually, I started development in that way. For example: -DrunOrder=random:123456. But, that lead to some dirty code because RunOrder is a enum like class, and it's hard to store additional data there. I came up with specific classes to hold randomization and I figure out that it is pointless to make RunOrder responsible for two tasks (mode and seed). This is way I decided that additional parameter will be better solution. It's also slightly easier to pass your seed in that way.
@cardil
-DrunOrder=random:123456 I meant as well.
The enums are only some model representation of real world but it does not mean that we cannot change it.
What we should not do is adding a new parameter, especially if we could reuse existing parameter and altering it's behavior and extendig the format of param value.
We have ambition in Surefire 3 to add parameters but in different way than this approach.
Maybe you was too fast and spent time on this code instead of asking for visions.
@joekienzle I need to read all this through after 2 years. @cardil Can you meanwhile rebase on master and resolve the git conflicts? Thx.
I've:
- merged the code with upstream,
- change the mechanism to utilize existing runOrder parameter like:
-DrunOrder=random:123456 - added some tests to cover the changes
- fixed some integration tests that ware failing or behave flaky
Please review @Tibor17
@cardil You have several commits in PR. Can you squash them in one? This simplifies the review, Thx.
@cardil
I am currently running all tests in your branch.
Did you also run the build mvn install -P run-its and checked that all tests passed?
Yup. I run them all multiple times. In fact I correct flaky ones. All should pass.
I have observed these build errors. How is it related to the work of random exec in this PR?
Tests in error:
testRandomWithSetInPomAndSeed123456(org.apache.maven.surefire.its.RunOrderParallelForksIT): Does not contain tests in sequence: [TA, TB, TC]
testRandomWithSeed123456(org.apache.maven.surefire.its.RunOrderParallelForksIT): Does not contain tests in sequence: [TA, TB, TC]
testRandomWithSeed654321(org.apache.maven.surefire.its.RunOrderParallelForksIT): Does not contain tests in sequence: [TC, TB, TA]
testRandomWithSetInPomAndSeed123456(org.apache.maven.surefire.its.RunOrderIT): Does not contain tests in sequence: [TA, TB, TC]
testRandomWithSeed123456(org.apache.maven.surefire.its.RunOrderIT): Does not contain tests in sequence: [TA, TB, TC]
testRandomWithSeed654321(org.apache.maven.surefire.its.RunOrderIT): Does not contain tests in sequence: [TC, TB, TA]
Tests run: 770, Failures: 0, Errors: 6, Skipped: 138
Yup. It's the right branch. Those tests are ones that I added. They test if running with given seed will produce repeatable build. Ideally it should produce the same outcome on every machine thus those tests I've added. I will dig in to this to determine a fault.
I've manage to reproduce those errors on different machine, with Windows 10. Running it from IT test fails with errors you just posted. Maybe forked launcher somehow involves?!
I will dig into more and keep you posted.
Ok. I've manage to fix this bug. Tests are now passing on Linux and Windows on multiple JDK: Azul, Oracle and OpenJDK.
The error was that test randomization was done on list of loaded classes, but class loading doesn't provide a contract on order of loading. So, input list was platform dependent. I've added pre step of sorting input class list. In that way randomization with seed is predictable, every time on every platform.
https://github.com/apache/maven-surefire/pull/112/files#diff-5bf860cde9189b3ffb9b3e0295282663R42
@Tibor17 please review again
Sorry, I had to work on making stable build with Java 11 and issues which came up after accepting SUREFIRE-1535. It seems I have got success.
On Wed, Sep 5, 2018 at 11:18 PM Krzysztof Suszyński < [email protected]> wrote:
@Tibor17 https://github.com/Tibor17 Please review
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/maven-surefire/pull/112#issuecomment-418884789, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_yR34zrWUi3qKvf_PNSUJ5k0SXz8A-ks5uYD-agaJpZM4IHvcI .
-- Cheers Tibor
This feature looks interesting. @cardil please note, that your PR has conflicting files.
This code review is taking eons to move in any direction.
If there is someone willing to review and merge this I will fix those conflicts. I hope this will not freeze again.
Ok. I've rebased the code.
All tests passing. I've executed command:
mvn clean install -Prun-its -e -V -nsu
with Maven 3.6.0, and OpenJDK 8u202 running on Ubuntu 18.04
@cardil Thx for the progress. In #226 we have a tendency not to push users to write a code for surefire nothing but exposing Extensions API which can be implemented by the same user in his POM without asking us in ASF. Nevertheless we should adopt your patch but as an extension which prevents from long waiting periods of our acceptance.
@Tibor17 Should I do something with this PR? Rewrite it somehow? I do not understand.
@Tibor17 Should I implement the new extenstion API introduced in https://github.com/apache/maven-surefire/pull/232 or just fix the conflicts?
@cardil Yes please resolve these conflicts. I saw your changes on GitHub already but I have to see them better again in IDE. The runorder is calculated in the forked jvm, so it means the extension won't be possible.