12rcu commented on PR #4990: URL: https://github.com/apache/ignite-3/pull/4990#issuecomment-2571750197
@sashapolo In retrospect, it may not be the best name for the pull request, in my opinion the "fix" is not that the test will always succeed, but rather gives a better reason why it failed. Yes, the timeout changes the semantics of the test, the first loops technically have more time than the other loops, and this could change the behaviour of the test. In my opinion this is negligible in this test case as we want the test to always succeed and never timeout (so if we wait longer for some loops then others, it doesn't really change much). So why would I want to use the timeout annotation, even though it's semantically not the same: - The exception that was thrown with the `waitForCondition()` leads to an assertion error, which I think is bad and would be clearer if a timeout was clearly stated. - The timeout of the test can be more easily changed if needed in the future. - I would like to see more use of the @Timeout annotation in the tests, and this would be my way of "fixing" other flaky tests in future submissions. But I am also happy to test the timeouts with the `waitForCondition()` and find better timeout values. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
