-
Notifications
You must be signed in to change notification settings - Fork 19
Fix/address flaky tests #289
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/address flaky tests #289
Conversation
…to avoid flakiness when running via VS Code GUI
CodSpeed Performance ReportMerging #289 will not alter performanceComparing Summary
Footnotes
|
seddonym
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. Would you mind removing the fuzz test tooling though, not convinced we need it yet. (If you want to put it in a separate PR in case I change my mind, feel free!)
| dirname = os.path.dirname(__file__) | ||
| filename = os.path.abspath( | ||
| os.path.join(dirname, "..", "assets", "syntaxerrorpackage", "foo", "one.py") | ||
| filename = str( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, why does this change fix the issue? They look equivalent to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are differences in behaviour between os.path.abspath and pathlib.resolve(). Specifically (in this case) the latter will normalize Windows drive letter names to capitalize them, e.g. c:\ -> C:\. There are differences with unix too: abspath will only make the path absolute, e.g. it won't apply resolutions (e.g. symlinks etc.).
It looks like build_graph already applies this normalization, so to get the test to match, we need to match the exact string including capitalization.
Without the fix:
__file__ == 'c:\\Users\\namc\\Repositories\\grimp\\tests\\functional\\test_error_handling.py'
filename == 'c:\\Users\\namc\\Repositories\\grimp\\tests\\assets\\syntaxerrorpackage\\foo\\one.py'Versus with the fix
__file__ == 'c:\\Users\\namc\\Repositories\\grimp\\tests\\functional\\test_error_handling.py'
filename == 'C:\\Users\\namc\\Repositories\\grimp\\tests\\assets\\syntaxerrorpackage\\foo\\one.py'When running pytest from the CLI/Just, the VS Code console uses capital C:\ for the current directory (so tests pass) but when running from the VS Code test-running UI, by default the --root-dir argument is set using a non-normalized path with lower-case c:\, and the test was failing.
['-p', 'vscode_pytest', '--rootdir=c:\\Users\\namc\\Repositories\\grimp', '--capture=no', 'c:\\Users\\namc\\Repositories\\grimp\\tests\\functional\\test_error_handling.py::test_syntax_error_includes_module']
justfile
Outdated
| # Fuzz Python test pollution. | ||
| [group('testing')] | ||
| fuzz-test-pollution-python: | ||
| @uv run detect-test-pollution --fuzz --tests ./tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, cool tool, though less sure about adding it here, as it's extra code / dependencies to maintain. If someone chooses to do this (as you did) there is nothing stopping them.
(Incidentally - would this be better if it took an argument of the test that is flaking?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted it.
There are two options, detect-test-pollution --fuzz to find polluted tests automatically, and detect-test-pollution --failing-test which lets you specify a specific test to find out why it is being polluted. Typically you just run the former initially and keep working through each polluting test one by one.
Here is a typical view of the output, for reference sake
Details
> uv run detect-test-pollution --fuzz --tests ./tests
discovering all tests...
-> discovered 852 tests!
run 1...
-> found failing test!
try `detect-test-pollution --failing-test tests/functional/test_build_and_use_graph.py::TestFindDownstreamModules::test_as_package_true --tests ./tests`!
>
> uv run detect-test-pollution --failing-test tests/functional/test_build_and_use_graph.py::TestFindDownstreamModules::test_as_package_true --tests ./tests
discovering all tests...
-> discovered 852 tests!
ensuring test passes by itself...
-> OK!
ensuring test fails with test group...
-> OK!
running step 1:
- 851 tests remaining (about 10 steps)
running step 2:
- 425 tests remaining (about 9 steps)
running step 3:
- 212 tests remaining (about 8 steps)
running step 4:
- 106 tests remaining (about 7 steps)
running step 5:
- 53 tests remaining (about 6 steps)
running step 6:
- 27 tests remaining (about 5 steps)
running step 7:
- 13 tests remaining (about 4 steps)
running step 8:
- 7 tests remaining (about 3 steps)
running step 9:
- 3 tests remaining (about 2 steps)
double checking we found it...
-> the polluting test is: tests/unit/adaptors/test_caching.py::TestCacheFileNamer::test_names_meta_cache_per_package
451dae5 to
89e1039
Compare
Resolves #288