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

Add coverage for making calls with incorrect arguments #1072

Merged
merged 13 commits into from
Mar 19, 2025

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Mar 18, 2025

What do these changes do?

Add coverage for making calls with incorrect arguments

Are there changes in behavior for the user?

no

@bdraco bdraco added the bot:chronographer:skip This PR does not need to include a change note label Mar 18, 2025
@bdraco bdraco force-pushed the test_bad_arg_calls branch from 81ff775 to 0a6c8bd Compare March 18, 2025 22:35
Copy link

codspeed-hq bot commented Mar 18, 2025

CodSpeed Performance Report

Merging #1072 will improve performances by 16.97%

Comparing test_bad_arg_calls (f722d6c) with master (4445017)

Summary

⚡ 3 improvements
✅ 35 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
test_cimultidict_extend_istr 143.4 µs 130.6 µs +9.84%
test_cimultidict_extend_str 198.4 µs 169.6 µs +16.97%
test_multidict_extend_str 198.9 µs 170.9 µs +16.37%

Copy link

codecov bot commented Mar 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.68%. Comparing base (4445017) to head (f722d6c).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1072      +/-   ##
==========================================
+ Coverage   97.63%   97.68%   +0.05%     
==========================================
  Files          22       23       +1     
  Lines        2618     2679      +61     
  Branches      376      376              
==========================================
+ Hits         2556     2617      +61     
  Misses         35       35              
  Partials       27       27              
Flag Coverage Δ
CI-GHA 97.68% <100.00%> (+0.05%) ⬆️
MyPy 86.68% <88.52%> (+0.04%) ⬆️
pytest 98.31% <100.00%> (+0.02%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bdraco bdraco closed this Mar 18, 2025
@bdraco bdraco reopened this Mar 18, 2025
@bdraco bdraco marked this pull request as ready for review March 18, 2025 22:47
@bdraco bdraco requested a review from asvetlov as a code owner March 18, 2025 22:47
@webknjaz
Copy link
Member

Could you use parametrize or pytest-subtest so that the test report would show individual tests clearly?

@bdraco
Copy link
Member Author

bdraco commented Mar 19, 2025

switched it to use parametrize.. it does seem less readable though

@asvetlov
Copy link
Member

I'm neutral regarding explicit/parametrizing tests, but I definitely love to have these tests merged.

@webknjaz
Copy link
Member

switched it to use parametrize.. it does seem less readable though

@bdraco I see what you mean. That's probably because of the arg names. I guess I'd go for tested_call_positional_args and tested_call_keyword_args. These are longer but not meaningless in the context.

Also, detached IDs may be more difficult to map the more params there are. So it might be worth it wrapping them with pytest.param() so they are coupled. Or I'd use a trick another pytest maintainer shown me once and bundle params through a data class..

By the way, you could do this through a parametrized fixture instead of a decorator. Perhaps, it'd look nicer.

Additionally, there's the first line in each test that could go to a fixture too.

P.S. With generic/stdlib exceptions, things like pytest.raises() must always use the match= regexp. It's less of a problem with framework-specific exceptions but I like having it for those too. This is because the test may remain green when something unexpected starts raising this exception or its subclass.


I could add a couple of commits on top to see how you like it.

@asvetlov
Copy link
Member

Wow, now it looks much better!

@asvetlov
Copy link
Member

Heh, pypy runner fails now. Any ideas wht's going on?

@webknjaz
Copy link
Member

I'm not sure why https://github.com/aio-libs/multidict/actions/runs/13950577347/job/39049255131?pr=1072#step:15:84 is happening on PyPy. It doesn't seem related to my changes but restarting the job doesn't help. It seems to reference https://github.com/aio-libs/multidict/blob/bb350855d4c956a3cbd1b0b81ee56ff170188819/tests/test_version.py#L97C5-L97C20, which isn't being changed in this PR.
Can it be related to fixture scopes? 🤔

@webknjaz
Copy link
Member

The traceback contains pytest_collection_modifyitems which is a hook we use. I have a feeling like there might be some sort of race condition within pytest on PyPy.

@webknjaz
Copy link
Member

The traceback contains pytest_collection_modifyitems which is a hook we use. I have a feeling like there might be some sort of race condition within pytest on PyPy.

Just tested locally that commenting out our own hook doesn't help, and it still produces the exception.

@asvetlov
Copy link
Member

I think you could skip the whole file on pypy.
It is for testing C Extension anyway.

@webknjaz
Copy link
Member

It's a regression in pytest v8.2.2.

@webknjaz
Copy link
Member

I think you could skip the whole file on pypy. It is for testing C Extension anyway.

I'm not sure. It breaks during the test collection stage. I'll probably just restrict pytest in that env. But I'm still looking into it. I have a feeling that another workaround will be changing the scope to match the dependency fixtures. I'm trying to figure out that's the smallest reproducer for upstream, but I can't get past requiring several test modules in the command. pytest --collect-only --no-cov tests/test_abc.py tests/test_copy.py tests/test_incorrect_args.py tests/test_multidict.py tests/test_mypy.py tests/test_pickle.py tests/test_types.py tests/test_update.py tests/test_version.py reproduces it but removing any of those modules from CLI args makes it work.
I have a feeling that it might be an issue of PyPy's implementation of OrderedDict because with --pdb I discovered that it has a duplicate <Function test_setdefault[case-insensitive-pure-python-module]> in keys and I suspect that might be a reason for OrderedDict.move_to_end() raising a KeyError.

@webknjaz
Copy link
Member

I have a feeling that another workaround will be changing the scope to match the dependency fixtures.

Nope. That ain't it.

@webknjaz
Copy link
Member

Finally logged pytest-dev/pytest#13312. And confirmed that newer versions of PyPy can't act as a workaround either.

@webknjaz webknjaz mentioned this pull request Mar 19, 2025
2 tasks
bdraco added 4 commits March 19, 2025 21:20

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)
bdraco and others added 9 commits March 19, 2025 21:20

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)

Verified

This commit was signed with the committer’s verified signature.
webknjaz 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)
It's not very accurate since there are about 3 message templates. But
it's better than nothing.
@webknjaz webknjaz force-pushed the test_bad_arg_calls branch from dcb1e88 to f722d6c Compare March 19, 2025 20:27
@bdraco bdraco merged commit 0c42831 into master Mar 19, 2025
44 of 47 checks passed
@bdraco bdraco deleted the test_bad_arg_calls branch March 19, 2025 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:skip This PR does not need to include a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants