Skip to content

Added "RANDOM" method sorting method. #1204

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

Closed
wants to merge 1 commit into from
Closed

Added "RANDOM" method sorting method. #1204

wants to merge 1 commit into from

Conversation

mrnichols76
Copy link

Closes issue #1194.

This commit creates an additional method sorting option called "RANDOM," which uses a random number generator (java.util.Random) to cause JUnit to execute tests in random order. It also adds another annotation, "@seed," which allows the random number generator's seed to be set to a pre-defined value (so a previous random ordering can be applied repeatedly). If no "@seed" annotation is included, JUnit will generate a seed. In any event, if "RANDOM" ordering is applied, the seed used will be written to stdout so that a failed randomly ordered test run (even one using a JUnit-supplied seed) can be re-run in the same order.

The code uses the Knuth/Fisher-Yates shuffle algorithm to do the reordering.

@kcooney
Copy link
Member

kcooney commented Sep 27, 2015

Thanks for the contribution.

While some people might want the random seed specified in code, others would prefer to specify it via an environment variable or system property. Still others would want the random seed to be fixed for some period of time (perhaps a day) and change at regular intervals, so if a particular order causes a failure you have time to diagnose the cause and fix it.

IMHO, a random order that changes every time you run the test is quite problematic. It would be easy to be in the middle of a coding session, run the tests, notice the failure, and start undoing changes trying to find out what you did to "break" the tests, only to discover later that the breakage had nothing to do with your code.

For these changes and many more, I would strongly prefer an API that would allow people to shuffle tests that requires that the developer (or a third-party library) to specify the ordering. See, for example, pull #1130

* This sort option generates and displays (to stdout) a
* seed value for the random number generator.
*/
RANDOM(null),
Copy link
Member

Choose a reason for hiding this comment

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

BTW, if you want the ordering provided by RANDOM to be consistent when you pass the same seed, then you'll need to actually order the methods in a deterministic way before you do the shuffling.

Copy link
Author

Choose a reason for hiding this comment

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

I just pushed up a commit that fixes this (just had to change line 33 to use the DEFAULT ordering--since in MethodSorter, the existing line to do the sort is before the shuffle takes place). Thanks for the comments, by the way.

@kcooney
Copy link
Member

kcooney commented Sep 27, 2015

@mrnichols76 Just to be clear, I commented on some of the smaller issues in the code, but I still personally feel that this is not the best approach for solving this issue.

@marcphilipp what are your thoughts?

@mrnichols76
Copy link
Author

@kcooney The way you proposed in pull #1130 is more general/flexible (and I must admit that I had not seen it until you pointed it out to me). It's better in a lot of ways.

However this pull (1204) has the virtue of providing the requested randomization feature with minimal changes to the existing codebase, so I think it's still worth considering.

@marcphilipp
Copy link
Member

I'm afraid I won't have time to look at this and #1130 in depth before this weekend.

@cardil
Copy link

cardil commented Sep 29, 2015

I think that passing a seed as a @Seed annotation is not ok, it will require to change the code. The code should be unchanged especially if one is hunting a dependency bug in unit test. I prefer to use system option like rspec does. I was referring here #1194 (comment)

It will also should be settable by Maven and Gradle plugins, to be usable by developers.

Second thing is that I think that random method order sorting is ok, but I that most of dependency bugs in tests are between different test classes. It would be nice that single seed can be reused by all test classes in project and to set random method sorting as default.

Maybe better way would be to give Maven and Gradle chance to set seed externally?
What do you think?

@mrnichols76
Copy link
Author

@cardil I like the idea of passing it in externally. As you say, it would be more useful if you could apply the same seed to multiple files. I hadn't thought about that when I wrote my code, mostly because I was thinking about the way that my students and I use JUnit (I teach Java programming). Do you think it would be better to use an environment variable for this or a command-line option? (or maybe both?)

I still think there ought to be an option to allow someone to put the seed in a test's annotation, though, because some users of IDEs may not want to bother tinkering with the details of build files or environment variables. Having a "@seed" annotation is like adding "@ignore" tag to a test--you can switch it on or switch it off. Yes, it changes the test's source file, but it doesn't really change the code of the test.

@kcooney
Copy link
Member

kcooney commented Sep 29, 2015

These kinds of reasonable debates on how to pick and override the seed are exactly why I would prefer an extension point. Let third-party library developers design an API and work with integration with build tools (for selecting and overriding the seed, as well as possibly including the seed in the test report).

@kcooney
Copy link
Member

kcooney commented Sep 18, 2016

I propose we close this. I still stand by my concerns I stated earlier.

@marcphilipp
Copy link
Member

Agreed.

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.

4 participants