Just to make sure I understand the proposal correctly: is the proposal to disallow the usage of @Test(timeout=...) for Flink Junit tests?
Here is my understanding of the pros/cons according to the discussion so far. Pros of allowing timeout: 1) When there are tests that are unreasonably slow, it helps us catch those tests and thus increase the quality of unit tests. 2) When there are tests that cause deadlock, it helps the AZP job fail fast instead of being blocked for 4 hours. This saves resources and also allows developers to get their PR tested again earlier (useful when the test failure is not relevant to their PR). Cons of allowing timeout: 1) When there are tests that cause deadlock, we could not see the thread dump of all threads, which makes debugging the issue harder. I would suggest that we should still allow timeout because the pros outweigh the cons. As far as I can tell, if we allow timeout and encounter a deadlock bug in AZP, we still know which test (or test suite) fails. There is a good chance we can reproduce the deadlock locally (by running it 100 times) and get the debug information we need. In the rare case where the deadlock happens only on AZP, we can just disable the timeout for that particular test. So the lack of thread dump is not really a concern. On the other hand, if we disallow timeout, it will be very hard for us to catch low-quality tests. I don't know if there is a good alternative way to catch those tests. On Mon, Apr 26, 2021 at 3:54 PM Dawid Wysakowicz <dwysakow...@apache.org> wrote: > Hi devs! > > I wanted to bring up something that was discussed in a few independent > groups of people in the past days. I'd like to revise using timeouts in > our JUnit tests. The suggestion would be not to use them anymore. The > problem with timeouts is that we have no thread dump and stack traces of > the system as it hangs. If we were not using a timeout, the CI runner > would have caught the timeout and created a thread dump which often is a > great starting point for debugging. > > This problem has been spotted e.g. during debugging FLINK-22416[1]. In > the past thread dumps were not always taken for hanging tests, but it > was changed quite recently in FLINK-21346[2]. I am happy to hear your > opinions on it. If there are no objections I would like to add the > suggestion to the Coding Guidelines[3] > > Best, > > Dawid > > > [1] https://issues.apache.org/jira/browse/FLINK-22416 > > [2] https://issues.apache.org/jira/browse/FLINK-21346 > > [3] > > https://flink.apache.org/contributing/code-style-and-quality-java.html#java-language-features-and-libraries > > >
