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.

Sign up for GitHub

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

fix(#4922): Fix flaky TestHealthTrait #5346

Merged
merged 1 commit into from
Apr 10, 2024
Merged

fix(#4922): Fix flaky TestHealthTrait #5346

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions e2e/common/traits/health_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,8 @@ func TestHealthTrait(t *testing.T) {

g.Eventually(IntegrationPodPhase(t, ctx, ns, name), TestTimeoutLong).Should(Equal(corev1.PodRunning))
g.Eventually(IntegrationPhase(t, ctx, ns, name), TestTimeoutShort).Should(Equal(v1.IntegrationPhaseRunning))
// Wait for the integration condition to become ready=false and then check that it remains not ready for some time - fixes some test flakiness
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 that normally, the Ready condition should start with a false value. So, this verification may hide the problem. I think we need to understand the root cause, and, if it's not already the case, initialize a Ready condition as false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can find my analysis of the root cause in #5345. It is the Deployment ready state that causes the intermediate Integration condition status ready=true. This is before the health trait is able to set the condition status to false.

My intention in this PR is to fix the flaky E2E test. In case we are not happy with the intermediate condition status ready=true I'd suggest to open a new issue and discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

The point is that, according to your analysis, the test is not wrong. What's wrong is the logic where we are blindly setting a value to something which should not be. If we change this test we'd be promoting a wrong behavior turning the bug into a feature. I am planning to retake the work of #5096 for next release which may intersect with this problem. I'd suggest to either work on the root cause or keep this on hold for the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you say implies that the logic has changed recently and the E2E test has become flaky due to that regression recently. I think the flakiness in this test exists for quite some time since #4922 is reporting it already on Camel K 2.1 and it may be flaky even before that.

I am fine with working on the root cause and fix the condition status setting but let's open a new issue now to track that in particular and let's not keep the flaky test until this is tackled and completely resolved. In a few weeks nobody will remember that the flakiness in this E2E test points to a misbehavior that should be fixed. We need a new issue for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this behavior exists since long time, also in 1.x branches probably. Reason why I advocate for working on the root cause. In any case, it's just my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #5351 to track the ready condition status flaw

g.Eventually(IntegrationConditionStatus(t, ctx, ns, name, v1.IntegrationConditionReady), TestTimeoutShort).Should(Equal(corev1.ConditionFalse))
g.Consistently(IntegrationConditionStatus(t, ctx, ns, name, v1.IntegrationConditionReady), 1*time.Minute).
Should(Equal(corev1.ConditionFalse))
g.Eventually(IntegrationPhase(t, ctx, ns, name), TestTimeoutLong).Should(Equal(v1.IntegrationPhaseError))
Expand Down