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

Optional global timeout via system property #772

Closed

Conversation

reinholdfuereder
Copy link
Contributor

Ideas behind this initial commit for discussion/review on global timeouts are:
(1) I want to make sure that the set of unit tests really only consists of unit tests, i.e. they need to be very fast (to be executable in each incremental build), so I would like to set a global timeout (certainly not on each single test class or test case) of e.g. 50ms (in fact it should be of course even smaller, but one should scare all devs off right away...).
(2) To avoid very long builds or even infinitely stuck builds due to very slow or even infinitely stuck tests by just setting a tolerant maximum timeout: again I only want to do that once (globally) for all the tests in a build.

Note that different global timeout strategy variants can be useful:
(a) should the global timeout conditionally override specific test timeouts that are higher then the global timeout? to make sure a maximum test duration is not exceeded; also allows to test that something really must be very quick
(b) should a specific timeout always override the global timeout? e.g. to avoid the need to increase the global timeout, just because a single test takes longer

There is a test for variant (a).

In fact these two variants could even be implement at once and with two distinct system properties as there meaning is quite good expressed with:
(a) global timeout
(b) default timeout (i.e. if there is a more specific one for a test class -- independent of being smaller or higher -- then it takes precedence)

Symbolic example usage (in e.g. Ant build script):

<junit ...>
...


What do you think?

- different global timeout strategy variants can be useful:
(a) should the global timeout conditionally override specific test timeouts that are higher then the global timeout: to make sure a maximum test duration is not exceeded; also allows to test that something really must be very quick
(b) should a specific timeout always override the global timeout: e.g. to avoid the need to increase the global timeout, just because a single test takes longer
Now with test for variant (a)
@kcooney
Copy link
Member

kcooney commented Nov 25, 2013

A few general thoughts. I haven't looked at the code.

We've generally not liked approaches where globals (like system properties) affect the behavior of tests. The concern have been 1) it isn't clear looking at the test code that there is a timeout (or other global behavior) being applied, and 2) the behavior of the test would be different in an IDE than in a build (unless, of course, your IDE users specify the system property).

Note that we recently added support in JUnitCore for parsing command line flags (currently used for filtering tests, but could be used for other things). So if having global settings for timeouts made sense, using command line flags would be more consistent than using system properties.

If you have control over the main class, I believe this behavior could be done by adding a command line flag that allowed you to attach a RunnerListener to the run. That would support test-run timeouts, but not method level timeouts.

Possibly relevant is recent discussions to allow package-level annotations to affect behavior of tests.

At my work, we have test-run level timeouts (implemented in the code that launches the JVM) and we have a listener that writes an XML format that is similar to Ant but includes detailed timing information.

@reinholdfuereder
Copy link
Contributor Author

@kcooney Thanks for your fast feedback and your pointers/hints.

While I see and share your concerns, I must admit that my position is kind of pragmatic and paranoid at the same time: (a) I want to avoid that devs need to consider anything that should not bother them (like having to use a special test runner, or adding timeouts) for normal plain unit tests, (b) especially if this is very easily just forgotten and would at least only pop up very late, if at all (e.g. if being detected by manual checks), or would require redundant/duplicate dedicated additional automation checks, (c) of course local integration at a devs machine must always mean (incremental) building of the project with the same build system (infrastructure) than on the build server, and that of course includes the execution of all unit tests.

After spending a little bit of time investigating the sources of our build system (gradle), I came to the conclusion that there is just no (elegant and/or maintainable and/or non-duplicating) way to using command line flags (yet). To be honest I would also not know how to nicely implement the multiple handing over of these timeouts through the "layers" of the JUnit system (core, request, runner) so that each runner is supporting it.

I also had a look at the RunListener (how do you actually manage this special formatting in your posting: is there a page with the supported Wiki syntax?) and so I think I understand what you mean with "[...] would support test-run timeouts, but not method level timeouts": however, I really want timeouts per test case (e.g. to identify non-unit JUnit test cases).

Package level annotations are also not what I would like to use, as this is in my opinion just unmaintainable for these purposes.

Maybe the following provides an additional thought on why I am thinking of this approach and I still think this is a pragmatic one for the real world: Let's say we are not starting a greenfield project and so there is quite some code base (including tests based on JUnit: please note that I don't call them unit tests, as they IMHO consist of real unit tests, but also of kind of big/slow/... integration tests) already there. So at first we should not be too harsh to the devs by setting a too stringent timeout per test case, as otherwise they get frustrated or even unmotivated to write any unit tests at all anmyore. There is also hope that due to getting rid or refactor or improve these big/slow/... non-unit tests, the devs actually are getting happier, because their local building and integration (including of course running all the unit tests) is significantly faster and makes them more productive. And so they actually see the benefit of real unit tests. When this has happened, one would hope that decreasing the timeout for unit tests again (to converge to acceptable real unit test time needs of a few milliseconds, so maybe this should be a several decrease step procedure) is even embraced and welcome by the devs...

Of course the introduction of big/slow/... non-unit integration tests can and typically really will also always happen in real world projects, I think.

@reinholdfuereder
Copy link
Contributor Author

@kcooney Please note that I have now also posted my concern and your pointer/hint on JUnitCore cmd line parsing on the gradle forum: http://forums.gradle.org/gradle/topics/timeouts_in_junit_tests

@kcooney
Copy link
Member

kcooney commented Feb 4, 2014

I think a possible way forward for requests like this is to provide APIs so developers can do what they want. We've had a few requests for global behavior (configured by system properties or something else). The maintainers of JUnit have been a bit uncomfortable to bake that behavior into the JUnit core, but we support the notion that JUnit should have good extension points.

I'm personally leading towards using http://docs.oracle.com/javase/6/docs/api/java/util/ServiceLoader.html as an extension mechanism. We could have JUnit provide extension interfaces, and end users could define their own implementations, and even release them to the public.

In this specific example, we could provide a hook that would allow an extension to add additional rules.

The beauty of the ServiceLocator mechanism is that, unlike the proposals to use command line flags, you don't need to control the way the tests are started (IDE, build tool, JUnitCore, etc). You just need to package your extension.

One possible problem with ServiceLoader is that it was introduced by JDK 6, and currently JUnit only requires JDK 5. JDK5 reached it's end of life in October 2009 (see http://www.oracle.com/technetwork/java/eol-135779.html), so I don't
think it's unreasonable for us to say that JUnit requires at least JDK6 (Android even supports JDK6). We just need to pick which release to make the change.

@reinholdfuereder
Copy link
Contributor Author

Thanks for this great idea: I may have missed the point of your comments before (cf. #768 (comment))

@reinholdfuereder
Copy link
Contributor Author

@kcooney so I think this pull request should be closed (with won't fix), because you don't want global system properties. and once I start with your suggested approach above, I will start another pull request. What do you think?

@kcooney
Copy link
Member

kcooney commented Mar 27, 2014

@reinholdfuereder makes sense.

@kcooney kcooney closed this Mar 27, 2014
@reinholdfuereder
Copy link
Contributor Author

Thanks

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.

None yet

2 participants