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

Make TestDNSResolver more reliable #3088

Merged
merged 4 commits into from
May 25, 2023
Merged

Conversation

imiric
Copy link
Contributor

@imiric imiric commented May 23, 2023

Instead of relying on log messages, this hooks into the DNS resolver, and tracks how many times a resolution attempt was made. This should be more reliable, and avoid the recent flakiness we've seen with this test on Windows, even after #1974:

scheduler_ext_test.go:1130:
        Error Trace:    D:/a/k6/k6/execution/scheduler_ext_test.go:1130
        Error:          Should be true
        Test:           TestDNSResolver/cache/3s
        Messages:       expected error to contain one of the list of messages

@imiric imiric force-pushed the test/fix-testdnsresolver-flakiness branch from 61a5e88 to 138b6fe Compare May 23, 2023 10:49
@imiric
Copy link
Contributor Author

imiric commented May 23, 2023

Nope, checking samples seems to be even flakier than checking logs... 😮‍💨 I guess we need to remove the reliance on timing altogether. 🤔

Or maybe we should get rid of the exact samples count, and just assert that we got at least one failed request? That kind of works against the point of checking the TTL, though. :-/

Ivan Mirić added 4 commits May 24, 2023 19:57
It was only used in TestDNSResolverCache, and it's really not relevant
for this specific test, and it was only harming us before since the
error messages were different on every OS. Since other tests don't use
it, let's just remove it.
There were probably more subtests here at some point that got
shuffled/removed, so this can be a main test now.
This will help us inspect the behavior of the cache.
Instead of relying on log messages, this hooks into the DNS resolver,
and tracks how many times a resolution attempt was made after that.
This should be more reliable, and avoid the recent flakiness we've seen
with this test on Windows[1], even after #1974:

scheduler_ext_test.go:1130:
        Error Trace:    D:/a/k6/k6/execution/scheduler_ext_test.go:1130
        Error:          Should be true
        Test:           TestDNSResolver/cache/3s
        Messages:       expected error to contain one of the list of messages

[1]: https://github.com/grafana/k6/actions/runs/5047828671/jobs/9071063454
@imiric imiric force-pushed the test/fix-testdnsresolver-flakiness branch from 138b6fe to 1bd7109 Compare May 24, 2023 18:05
@codecov-commenter
Copy link

Codecov Report

Merging #3088 (c76a18f) into master (c0aff92) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head c76a18f differs from pull request most recent head 1bd7109. Consider uploading reports for the commit 1bd7109 to get more accurate results

@@            Coverage Diff             @@
##           master    #3088      +/-   ##
==========================================
- Coverage   75.33%   75.27%   -0.06%     
==========================================
  Files         232      232              
  Lines       17592    17593       +1     
==========================================
- Hits        13253    13244       -9     
- Misses       3488     3498      +10     
  Partials      851      851              
Flag Coverage Δ
ubuntu 75.27% <100.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/testutils/mockresolver/resolver.go 72.72% <100.00%> (-12.99%) ⬇️

... and 3 files with indirect coverage changes

@imiric
Copy link
Contributor Author

imiric commented May 24, 2023

Hey @codebien @olegbespalov, please see the latest force push. I abandoned checking samples, since I didn't want to depend on metric emission for the functionality of the test, and using MiniRunner would make the test less useful, since the integration with js.Runner is important.

Adding a small hook to MockResolver avoids the time-sensitive behavior, so it should hopefully be more reliable now. Let me know.

@imiric imiric changed the title Maybe fix TestDNSResolver flakiness on Windows Make TestDNSResolver more reliable May 24, 2023
Copy link
Collaborator

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Left a small non-blocking comment, but generally looks good 👍

Let's merge it and make our tests trusty again 💪 😅

lib/testutils/mockresolver/resolver.go Show resolved Hide resolved
Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Well done! This is for sure a better solution. 👏

execution/scheduler_ext_test.go Show resolved Hide resolved
@imiric imiric merged commit 06e1628 into master May 25, 2023
@imiric imiric deleted the test/fix-testdnsresolver-flakiness branch May 25, 2023 10:19
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.

None yet

4 participants