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

Test fixes #5415

Merged
merged 6 commits into from
May 8, 2024
Merged

Test fixes #5415

merged 6 commits into from
May 8, 2024

Conversation

valdar
Copy link
Member

@valdar valdar commented Apr 24, 2024

Some fixes mostly to prevent flaky tests run. It is part of the work done in #5119

@valdar valdar requested a review from christophd April 24, 2024 11:24
var TestTimeoutShort = 1 * time.Minute
var TestTimeoutMedium = 5 * time.Minute
var TestTimeoutLong = 15 * time.Minute
var TestTimeoutShort = 5 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these values are too high. If we need to change them specifically for certain tests, then, we should change the test and include the specific timeout instead.

Copy link
Member Author

@valdar valdar Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my empirical tests these are the values that minimize test runs failing due to small perturbations in load or network speed. Honestly I don't see may cons in bumping them, the only one is that actually failing tests will take longer. But that is a condition that should happens rarely since most of the runs should be of passing tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly the problem. In case of an unstable change we may introduce, the time to complete a test would be at least duplicated, having to wait for a feedback too much time. Imho, the goal should be to have faster timeouts. If some test is not stable, then, I don't think the culprit is the timeout. And if it's the timeout, likely it's in the specific flaky test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see both of you having a point here. IMO if the increased timeout really fixes the flaky tests I tend to accept the fact that we need to wait some more time when a test really is broken by the changes in a PR. The feedback loop for a successful test is over 60 minutes anyways and having a flaky test can be really exhausting.

@valdar valdar added the trigger native test Use this label in PR when you want to trigger Quarkus Native tests label Apr 24, 2024
@christophd
Copy link
Contributor

I think the native tests tend to not work in GH actions, do they? @valdar Maybe you can run the native tests on your machine and give feedback here when they succeeded?

@valdar valdar merged commit 6da16e2 into apache:main May 8, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trigger native test Use this label in PR when you want to trigger Quarkus Native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants