You're right - changing this timeout wouldn't help. There are 2 timeouts for test execution time: one in surefire and another is in junit. Surefire just kills a child process when timeout is exceeded, and this patch doesn't handle this.
Got it. Makes sense. Thanks a lot for the explanation.
What's handled is if a test method is annotated with @Test and the timeout parameter is given, then junit will fail the test and thread dump will be printed. TestFileAppend4 is an example of a test providing timeout parameter.
Gotcha. I also tested this similarly and was able to confirm that the patch works as expected. Very cool.
AFAIK we can't really do anything with the surefire timeout. Still we may have thread dumps printed for all tests in case of timeouts if we introduce a default timeout for all tests on the junit level. I guess it is doable with a custom surefire provider for junit, but I'm not sure we really need this. What do you think?
I think that would be a great thing to add. Not very many of the Hadoop tests are annotated with a JUnit timeout (79 / 3984, by my quick count), and in my experience tests which aren't annotated with a JUnit timeout certainly do in fact timeout and end up hitting the Surefire fork timeout. If that's not handled somehow, that would make this change much less useful. If it's easy, mind adding a default JUnit timeout to the next rev of this patch? If not, we could certainly do it as a separate JIRA.
As for comments on the contents of the patch, just a few little things:
- I think it'd be good to print an informative header before dumping the stacks, e.g. something like "====> TEST TIMED OUT. PRINTING THREAD DUMP. <===="
- Several of the lines in the patch go over 80 characters.
- Our coding guidelines say that lines which continue onto the next line should be indented 4 spaces, not 2.
Otherwise the patch looks great.