Skip to content

Conversation

@kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Jun 23, 2022

at module import time, the REDIS_INFO dict hasn't been initialized.

Pull Request check-list

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

  • Does $ tox pass with this change (including linting)?
  • 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?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

when running the unittests manually, e.g. with pytest tests from the command line, test discovery would fail because the
REDIS_INFO dict, although existing, hadn't been initialized at import time. The skip tests need to evaluate the REDIS_INFO as initialized during session setup, hence the need for a late init (done via skip string)

Additionally, changed two instances of

assert round(time1) == round(time2)

to

assert abs(time1 - time2) < 1.0

The former will fail, if for example, time1==1000.4999 and time2==1000.5001 which is clearly not what is intended.

at module import time, the REDIS_INFO dict hasn't been initialized.
@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2022

Codecov Report

Merging #2248 (4e7a3a9) into master (6da8086) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@ ## master #2248 +/- ## ======================================= Coverage 91.85% 91.86% ======================================= Files 108 108 Lines 27776 27787 +11 ======================================= + Hits 25515 25527 +12  + Misses 2261 2260 -1 
Impacted Files Coverage Δ
tests/test_asyncio/conftest.py 93.38% <ø> (ø)
tests/conftest.py 86.50% <100.00%> (+0.05%) ⬆️
tests/test_asyncio/test_timeseries.py 100.00% <100.00%> (ø)
tests/test_timeseries.py 100.00% <100.00%> (ø)
redis/commands/core.py 82.11% <0.00%> (ø)
tests/test_cluster.py 97.08% <0.00%> (+0.01%) ⬆️
redis/cluster.py 90.33% <0.00%> (+0.02%) ⬆️
redis/asyncio/connection.py 84.07% <0.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6da8086...4e7a3a9. Read the comment docs.

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Jun 24, 2022

Unit-tests sporatically staling or failing is a big problem in this repo. This PR aims to fix that....

We have a stalled unittest somewhere, unfortunately, the logs are not quite up to identifying it...

platform linux -- Python 3.9.13, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 rootdir: /home/runner/work/redis-py/redis-py, configfile: tox.ini plugins: asyncio-0.18.3, cov-3.0.0, timeout-2.0.1 asyncio: mode=legacy collected 3961 items / 779 deselected / 3182 selected tests/test_bloom.py ................. tests/test_command_parser.py ...s. Error: The operation was canceled.``` This appears to be all of the tests in `test_command_parser.py`, so the hanging test must have something to do with the next file, test_commands.py. I'll try pushing this again and see if it goes through now. 
You can't test rounded values for equalness, since they may fall each on a different side of 0.5. It is better to test their absolute difference for a certain tolerance, in this case 1.0 which is the intent of the original round.
@kristjanvalur
Copy link
Contributor Author

so, yeah, spurious unit tests fail :)

@dvora-h dvora-h added the maintenance Maintenance (CI, Releases, etc) label Jun 27, 2022
@dvora-h dvora-h merged commit 8b18d5b into redis:master Jun 27, 2022
@dvora-h
Copy link
Collaborator

dvora-h commented Jun 27, 2022

@kristjanvalur Thank you for the fix!

@kristjanvalur
Copy link
Contributor Author

Uh-oh! I just noticed a problem in the pr. Please see #2253 for a follow-up fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Maintenance (CI, Releases, etc)

3 participants