Buck default java test runner may behave differently if --debug and --filter are specified or not

[romanoid]

https://github.com/facebook/buck/issues/2007 was one of the examples of such behavior observed (and fix fixed this behavior discrepancy in regards to some errors)

See this line: https://github.com/facebook/buck/blob/a6c7d7391a7a8e575c6372cac6690a240b5907ea/src/com/facebook/buck/testrunner/JUnitRunner.java#L221

This leads to using DelegateRunnerWithTimeout or not depending on if defaultTestTimeoutMillis is 0 or filter are specified.

Now --debug flag implies defaultTestTimeoutMillis.

This leads to both --filter and --debug changing test runner behavior, potentially causing discrepancies for test failures in normal "run everything" mode vs when you try to debug specific failure.

On the first glance i don't see why filters will not work with delegation using DelegateRunnerWithTimeout. In addition, not observing timeouts if only subset of tests are run may be potentially dangerous.

It is slightly more complicated with debug, we may want to keep timeout "infinite" when debugging, but retain other behaviors.

Potentially replacing defaultTestTimeoutMillis with Integer.MAX_VALUE rather than 0 may do the trick.

I'll be happy to investigate filtering and do the fixes if you think this approach is reasonable.

[bobyangyf]

Potentially replacing defaultTestTimeoutMillis with Integer.MAX_VALUE rather than 0 may do the trick. I think so too, but Integer.MAX is smaller than no limit at all. But I doubt it matters in most cases.

On the first glance i don't see why filters will not work with delegation using DelegateRunnerWithTimeout However, the comment above says that only "BuckBlockJUnit4ClassRunner" provides correct TestDescription for filtering. I'd be careful with changing that, but it probably is possible to provide the right description for all runners. Furthermore, "BuckBlockJUnit4ClassRunner" has timeout built into it, so we would not be "not observing timeouts if only subset of tests are run".

IMO There's probably more reasons to not need "DelegateRunnerWithTimeout" if "BuckBlockJUnit4ClassRunner" can do timeouts, since its strictly more powerful. But I'd be careful if there was some hidden behaviour required.

Feel free to look into this more and we can do reviews. But in the long run, we are probably looking to revamp the test infra and move to junit5.

[bobyangyf]

buck2