Skip to content

Conversation

@nathanjmcdougall
Copy link
Contributor

Resolves #288

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 18, 2025

CodSpeed Performance Report

Merging #289 will not alter performance

Comparing nathanjmcdougall:fix/address-flaky-tests (94e4421) with main (e4bb1bf)

Summary

✅ 26 untouched
⏩ 23 skipped1

Footnotes

  1. 23 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Collaborator

@seddonym seddonym 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 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(
Copy link
Collaborator

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.

Copy link
Contributor Author

@nathanjmcdougall nathanjmcdougall Dec 19, 2025

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
Copy link
Collaborator

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?)

Copy link
Contributor Author

@nathanjmcdougall nathanjmcdougall Dec 19, 2025

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

@seddonym seddonym merged commit 7fc49c4 into python-grimp:main Dec 20, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Address flaky tests

2 participants