intersphinx: fix flipped use of intersphinx_cache_limit.
#12905
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Follow-up to sphinx-doc#12514. The previous patch properly took negative cache limits into account (which are meant to always keep the cache). However, in the process, it flipped the comparison between `intersphinx_cache_limit` and zero. The consequence was that the cache was always kept for positive values, and always refreshed for negative values. The provided unit test didn't catch the mistake because it merely checked the return value of `_fetch_inventory_group()`, which is True if the cache has been updated *successfully*. However, what should've been tested was whether we *tried* to update the cache. The problem was that the test attempted to fetch the remote inventory from a nonsense address, which is reasonable for a test. However, the fetch fails, as expected. And correctly Sphinx tells us that the cache wasn't updated, even though it *tried to*. The new test replaces the inner fetch function with a mock that always returns some data. This means that "did we try to update the cache" and "did we successfullt update the cache" are now the same. It also lets us query the mock directly whether it has been called or not. In addition, the test also has been parametrized over multiple values. This way, we test not only the first of the following cases of interest, but all three: 1. negative cache limit 2. positive cache limit, cache fresh 2. positive cache limit, cache stale
troiganto
force-pushed
the
flip-check-intersphinx-cache-limit
branch
from
62d71c0
to
65703ee
Compare
picnixz
reviewed
Sep 26, 2024
There was a problem hiding this comment.
I'm extremely sorry for the inconvenience. I should stop reviewing PRs and accepting them when I'm still sleepy or too tired. By the way, thank you for the tests (I am now tired so I'll still ask @AA-Turner to have a final check).
Closed
# Conflicts: # CHANGES.rst
AA-Turner
approved these changes
Oct 7, 2024
There was a problem hiding this comment.
Thank you Nico (@troiganto) for the excellent description and PR, sorry it took too long to review.
A
|
np, thanks for the maintenance work! contributing was a lot simpler than for other projects, so I actually had a great time solving this! |
|
I am sorry for introducing another bug when fixing a bug. Thanks for fixing this, I learned more unit test tips from this PR. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Subject: intersphinx: fix flipped use of
intersphinx_cache_limit.Feature or Bugfix
Purpose
Follow-up to #12514, which fixed one bug surrounding
intersphinx_cache_limit, but introduced another by flipping a comparison.Detail
The previous patch properly took negative cache limits into account (which are meant to always keep the cache). However, in the process, it flipped the comparison between
intersphinx_cache_limitand zero. The consequence was that the cache was always kept for positive values, and always refreshed for negative values.The provided unit test didn't catch the mistake because it merely checked the return value of
_fetch_inventory_group(), which is True if the cache has been updated successfully. However, what should've been tested was whether we tried to update the cache.The problem was that the test attempted to fetch the remote inventory from a nonsense address, which is reasonable for a test. However, the fetch fails, as expected. And correctly Sphinx tells us that the cache wasn't updated, even though it tried to.
The new test replaces the inner fetch function with a mock that always returns some data. This means that "did we try to update the cache" and "did we successfullt update the cache" are now the same. It also lets us query the mock directly whether it has been called or not.
In addition, the test also has been parametrized over multiple values. This way, we test not only the first of the following cases of interest, but all three:
Relates