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

Fix flaky test Test_runDispatcher/gateway_actions_passed #5405

Merged

Conversation

andrzej-stencel
Copy link
Contributor

What does this PR do?

Fixes flaky test Test_runDispatcher/gateway_actions_passed by increasing the timeout from 100ms to 200ms.

The flakiness is a result of a 100 miliseconds not always being enough time to dispatch the action in the test environment.

The correct way to fix this issue is to run the assertions in an assert.Eventually with a longer timeout. Unfortunately, a deficiency in the testify testing framework makes it impossible to put the AssertExpectations methods in an assert.Eventually or assert.EventuallyWithT function: stretchr/testify#1414.

The workaround is to increase the context timeout for the gateway actions passed test case. I'm increasing it from 100ms to 200ms.

Why is it important?

Flaky tests are evil!

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] I have added an entry in ./changelog/fragments using the changelog tool
  • [ ] I have added an integration test or an E2E test

How to test this PR locally

$ go test -v ./internal/pkg/agent/application/ -run Test_runDispatcher

Related issues

A short context timeout makes some tests flaky, but making it longer across the board would unnecessarily make test runs longer. Let's make it customizable per test case.
The flakiness is a result of a 100 miliseconds not always being enough time to  dispatch the action in the test environment.

The correct way to fix this issue is to run the assertions in an `assert.Eventually` with a longer timeout. Unfortunately, a deficiency in the `testify` testing framework makes it impossible to put the `AssertExpectations` methods in an `assert.Eventually` or `assert.EventuallyWithT` function: stretchr/testify#1414.

The workaround is to increase the context timeout for the `gateway actions passed` test case. I'm increasing it from 100ms to 200ms.
@andrzej-stencel andrzej-stencel requested a review from a team as a code owner September 3, 2024 12:58
@andrzej-stencel andrzej-stencel changed the title Fix flaky test run dispatcher Fix flaky test Test_runDispatcher/gateway_actions_passed Sep 3, 2024
@andrzej-stencel andrzej-stencel changed the title Fix flaky test Test_runDispatcher/gateway_actions_passed Fix flaky test Test_runDispatcher/gateway_actions_passed Sep 3, 2024
Copy link
Contributor

mergify bot commented Sep 3, 2024

This pull request does not have a backport label. Could you fix it @andrzej-stencel? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip label Sep 3, 2024
Copy link

@ycombinator ycombinator added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Sep 3, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good.

@michel-laterman
Copy link
Contributor

There is discussion on the issue in the description that links to another issue, which links to stretchr/testify#1395; could the approach from this pr be used to make the Eventually type check work?

@andrzej-stencel
Copy link
Contributor Author

There is discussion on the issue in the description that links to another issue, which links to stretchr/testify#1395; could the approach from this pr be used to make the Eventually type check work?

@michel-laterman I don't know what approach you mean, can you elaborate?

Let me merge this pull request for now, but I'd be happy to continue the conversation to hopefully find a better solution.

@andrzej-stencel andrzej-stencel merged commit 0efab75 into elastic:main Sep 5, 2024
16 checks passed
@andrzej-stencel andrzej-stencel deleted the fix-flaky-test-run-dispatcher branch September 5, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flaky Test]: Test_runDispatcher/gateway_actions_passed – Should be empty, but was <insert address here>
5 participants