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

Fix resource usage and cleanup Mocks in the unit tests #2936

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Sep 12, 2023

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

Description of change

In pr #2911, we uncovered some problems in the unittests.

  • There were missing "await" calls in the tests for test_cwe_404.py. asyncio.wait() must not wait for coroutines directly (this will be banned in 3.11) but must use tasks.
  • Some AsyncMocks didn't use spec arguments and so methods, which weren't async, caused resource warnings. Mocking should use spec when possible.
  • Mocking done to Redis objects when simulating a cluster in nocluster mode was not done and so the test suite would attempt to return mock objects to a connection pool that had never issued them.

In addition, a fix to a permanently disabled Unix connection test is included.

@kristjanvalur kristjanvalur marked this pull request as ready for review September 12, 2023 19:48
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (1596ac6) 91.28% compared to head (78140b0) 91.31%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2936      +/-   ##
==========================================
+ Coverage   91.28%   91.31%   +0.03%     
==========================================
  Files         126      126              
  Lines       32398    32406       +8     
==========================================
+ Hits        29573    29592      +19     
+ Misses       2825     2814      -11     
Files Changed Coverage Δ
tests/conftest.py 86.08% <100.00%> (+0.05%) ⬆️
tests/test_asyncio/conftest.py 85.41% <100.00%> (+0.52%) ⬆️
tests/test_asyncio/test_cluster.py 96.93% <100.00%> (ø)
tests/test_asyncio/test_cwe_404.py 100.00% <100.00%> (ø)
tests/test_connect.py 96.99% <100.00%> (+9.77%) ⬆️

... and 4 files with indirect coverage changes

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

@chayim chayim added the maintenance Maintenance (CI, Releases, etc) label Sep 14, 2023
@dvora-h dvora-h merged commit 94ae3e0 into redis:master Sep 14, 2023
55 checks passed
@kristjanvalur kristjanvalur deleted the kristjan/unittest-fixes branch September 14, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants