Subject: intersphinx: fix flipped use of intersphinx_cache_limit.
Feature or Bugfix
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_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:
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