Skip to content

Increase flaky test sleep, replace deprecated assert#48417

Merged
vvoland merged 1 commit intomoby:masterfrom
adams1mon:try-fix-flaky-plugin-client-test
Oct 22, 2024
Merged

Increase flaky test sleep, replace deprecated assert#48417
vvoland merged 1 commit intomoby:masterfrom
adams1mon:try-fix-flaky-plugin-client-test

Conversation

@adams1mon
Copy link
Contributor

- What I did

  • tried to fix Flaky test: TestNewClientWithTimeout #47590
  • increased mock handler processing time to 50ms to try to prevent it from finishing before the 10ms client timeout occurs
  • replaced deprecated error type assertion

- How I did it

  • I tried to run the test with increasingly close processing and timeout times, flakiness occurred when the timeout and processing times were ~5ms apart on my machine

- How to verify it

  • I really don't know

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

- increase mock handler processing time to 50ms to try to prevent it from finishing before the 10ms client timeout occurs
- replace deprecated error type assertion

Signed-off-by: Adam Simon <adamsimon85100@gmail.com>
Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up!

For verifying whether the test is still flaky - you could add a new temporary commit that would modify the code so that the test code is ran like x100 times:

func TestNewClientWithTimeout(t *testing.T) {
    t.Parallel()
    for i := 0; i < 100; i++ {
        t.Run(strconv.Itoa(i), func(...) {
           <test body...>
        }
    }
}

@adams1mon
Copy link
Contributor Author

adams1mon commented Sep 3, 2024

Modified it to run the test 100 times. Before I also ran it with go test -count 100 multiple times to see if it fails, it seemed fine.

@adams1mon adams1mon requested a review from vvoland September 14, 2024 09:49
@vvoland vvoland added this to the 28.0.0 milestone Oct 16, 2024
@vvoland
Copy link
Contributor

vvoland commented Oct 16, 2024

Closing and reopening to re-run the CI.

@vvoland vvoland closed this Oct 16, 2024
@vvoland vvoland reopened this Oct 16, 2024
Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM (after removing the last commit 😄)

@adams1mon
Copy link
Contributor Author

Reverted the last commit, I hope the test won't fail anymore. I don't really know why it was failing in the first place, maybe the timeouts aren't that accurate and some additional waiting happens somewhere when the system is under load?
Although I think the millisecond range is pretty large for this kind of error to occur.

@adams1mon
Copy link
Contributor Author

The changed test passed, but some networkdb test (TestNetworkDBIslands) failed :(

@vvoland
Copy link
Contributor

vvoland commented Oct 21, 2024

The networkdb test is a known flaky, don't worry about it.

As for removing the commit - instead of reverting it with another commit, you can just remove it so the PR has only the first commit. You can do that with git rebase or git reset and then force push to the branch PR.

@adams1mon adams1mon force-pushed the try-fix-flaky-plugin-client-test branch from 2816ec1 to 28dc2f6 Compare October 21, 2024 16:06
@adams1mon
Copy link
Contributor Author

adams1mon commented Oct 21, 2024

Done (I thought the commits would be squashed on merge).

Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: TestNewClientWithTimeout

3 participants