intersphinx: fix flipped use of intersphinx_cache_limit.

Follow-up to #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

CHANGES.rst

@@ -102,6 +102,10 @@ Bugs fixed
 * #12844: Restore support for ``:noindex:`` for the :rst:dir:`js:module`
   and :rst:dir:`py:module` directives.
   Patch by Stephen Finucane.
+* #TBD: intersphinx: fix flipped use of :confval:`intersphinx_cache_limit`,
+  which always kept the cache for positive values, and always refreshed it for
+  negative ones.
+  Patch by Nico Madysa.
 
 Testing
 -------

sphinx/ext/intersphinx/_load.py

@@ -201,9 +201,13 @@ def _fetch_inventory_group(
     config: Config,
     srcdir: Path,
 ) -> bool:
-    if config.intersphinx_cache_limit < 0:
+    if config.intersphinx_cache_limit >= 0:
+        # Positive value: cache is expired if its timestamp is below
+        # `now - X days`.
         cache_time = now - config.intersphinx_cache_limit * 86400
     else:
+        # Negative value: cache is expired if its timestamp is below
+        # zero, which is impossible.
         cache_time = 0
 
     updated = False

tests/test_extensions/test_ext_intersphinx.py

@@ -745,30 +745,51 @@ def test_intersphinx_role(app):
 
 
 @pytest.mark.sphinx('html', testroot='root')
-def test_intersphinx_cache_limit(app):
+@pytest.mark.parametrize(
+    ('cache_limit', 'expected_expired'), [(5, False), (1, True), (-1, False)]
+)
+def test_intersphinx_cache_limit(app, monkeypatch, cache_limit, expected_expired):
     url = 'https://example.org/'
-    app.config.intersphinx_cache_limit = -1
+    app.config.intersphinx_cache_limit = cache_limit
     app.config.intersphinx_mapping = {
         'inv': (url, None),
     }
     # load the inventory and check if it's done correctly
     intersphinx_cache: dict[str, InventoryCacheEntry] = {
-        url: ('', 0, {}),  # 0 is a timestamp, make sure the entry is expired
+        url: ('inv', 0, {}),  # Timestamp of last cache write is zero.
     }
     validate_intersphinx_mapping(app, app.config)
-    load_mappings(app)
 
-    now = int(time.time())
+    # The test's `now` is two days after the cache was created.
+    now = 2 * 86400
+    monkeypatch.setattr('time.time', mock.Mock(name='time.time', return_value=now))
+
+    # `_fetch_inventory_group` calls `_fetch_inventory`. We replace it
+    # with a mock to test whether it has been called or not. If it has
+    # been called, it means the cache had expired.
+    mock_fetch_inventory = mock.Mock(return_value=('inv', now, {}))
+    monkeypatch.setattr(
+        'sphinx.ext.intersphinx._load._fetch_inventory', mock_fetch_inventory
+    )
+
     for name, (uri, locations) in app.config.intersphinx_mapping.values():
         project = _IntersphinxProject(name=name, target_uri=uri, locations=locations)
-        # no need to read from remote
-        assert not _fetch_inventory_group(
+        updated = _fetch_inventory_group(
             project=project,
             cache=intersphinx_cache,
             now=now,
             config=app.config,
             srcdir=app.srcdir,
         )
+        # If we hadn't mocked `_fetch_inventory`, it would've made
+        # a request to `https://example.org/` and found no inventory
+        # file. That would've been an error, and `updated` would've been
+        # False even if the cache had expired. The mock makes it behave
+        # "correctly".
+        assert updated == expected_expired
+    # Double-check: If the cache was expired, `mock_fetch_inventory`
+    # must've been called.
+    assert mock_fetch_inventory.called == expected_expired
 
 
 def test_intersphinx_fetch_inventory_group_url():