From d434bcf4fbd1e10bbdf9364fca65f04f7ca548bc Mon Sep 17 00:00:00 2001 From: Nicolas Simonds Date: Wed, 10 Jul 2024 17:44:42 -0700 Subject: [PATCH 1/6] fix: improve caching of parameterized fixtures The fix for Issue #6541 caused regression where cache hits became cache misses, unexpectedly. Attempt to restore the previous behavior, while also retaining the fix for the bug. Fixes: Issue #6962 --- AUTHORS | 1 + changelog/6962.bugfix.rst | 1 + src/_pytest/fixtures.py | 13 ++++++++++--- testing/python/fixtures.py | 31 +++++++++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 changelog/6962.bugfix.rst diff --git a/AUTHORS b/AUTHORS index 8d31170560c..9b6cb6a9d23 100644 --- a/AUTHORS +++ b/AUTHORS @@ -306,6 +306,7 @@ Nicholas Devenish Nicholas Murphy Niclas Olofsson Nicolas Delaby +Nicolas Simonds Nico Vidal Nikolay Kondratyev Nipunn Koorapati diff --git a/changelog/6962.bugfix.rst b/changelog/6962.bugfix.rst new file mode 100644 index 00000000000..9557d7b173e --- /dev/null +++ b/changelog/6962.bugfix.rst @@ -0,0 +1 @@ +Fixed bug where parametrized fixtures were not being cached correctly, being recreated every time. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 0151a4d9c86..cda4f0c42f3 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1056,9 +1056,16 @@ def execute(self, request: SubRequest) -> FixtureValue: my_cache_key = self.cache_key(request) if self.cached_result is not None: cache_key = self.cached_result[1] - # note: comparison with `==` can fail (or be expensive) for e.g. - # numpy arrays (#6497). - if my_cache_key is cache_key: + + # note: `__eq__` is not required to return a bool, and sometimes + # doesn't, e.g., numpy arrays (#6497). Coerce the comparison + # into a bool, and if that fails, fall back to an identity check. + try: + cache_hit = bool(my_cache_key == cache_key) + except (ValueError, RuntimeError): + cache_hit = my_cache_key is cache_key + + if cache_hit: if self.cached_result[2] is not None: exc, exc_tb = self.cached_result[2] raise exc.with_traceback(exc_tb) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 5c3a6a35b34..4968459fcb5 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1557,6 +1557,37 @@ def test_printer_2(self): result = pytester.runpytest() result.stdout.fnmatch_lines(["* 2 passed in *"]) + def test_parameterized_fixture_caching(self, pytester: Pytester) -> None: + pytester.makepyfile( + """ + import pytest + from itertools import count + + CACHE_MISSES = count(0) + + def pytest_generate_tests(metafunc): + if "my_fixture" in metafunc.fixturenames: + param = "d%s" % "1" + print("param id=%d" % id(param), flush=True) + metafunc.parametrize("my_fixture", [param, "d2"], indirect=True) + + @pytest.fixture(scope='session') + def my_fixture(request): + next(CACHE_MISSES) + + def test1(my_fixture): + pass + + def test2(my_fixture): + pass + + def teardown_module(): + assert next(CACHE_MISSES) == 2 + """ + ) + result = pytester.runpytest() + result.stdout.no_fnmatch_line("* ERROR at teardown *") + class TestFixtureManagerParseFactories: @pytest.fixture From a8ce305b485310f810693d15668b5c8b4fbdcc84 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Fri, 12 Jul 2024 08:05:00 -0300 Subject: [PATCH 2/6] Update testing/python/fixtures.py --- testing/python/fixtures.py | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 4968459fcb5..2761d9e19f4 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1558,6 +1558,7 @@ def test_printer_2(self): result.stdout.fnmatch_lines(["* 2 passed in *"]) def test_parameterized_fixture_caching(self, pytester: Pytester) -> None: + """Regression test for #12600.""" pytester.makepyfile( """ import pytest From bf5ca7b0cbc33a316c62e48ca6e7dc8c53e43f9d Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Fri, 12 Jul 2024 09:23:37 -0300 Subject: [PATCH 3/6] Improve test and comment --- src/_pytest/fixtures.py | 5 ++--- testing/python/fixtures.py | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index cda4f0c42f3..a55ccf3ed51 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1057,9 +1057,8 @@ def execute(self, request: SubRequest) -> FixtureValue: if self.cached_result is not None: cache_key = self.cached_result[1] - # note: `__eq__` is not required to return a bool, and sometimes - # doesn't, e.g., numpy arrays (#6497). Coerce the comparison - # into a bool, and if that fails, fall back to an identity check. + # Coerce the comparison into a bool (#12600), and if that fails, fall back to an identity check: + # `__eq__` is not required to return a bool, and sometimes doesn't, e.g., numpy arrays (#6497). try: cache_hit = bool(my_cache_key == cache_key) except (ValueError, RuntimeError): diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 2761d9e19f4..8d2646309a8 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1568,9 +1568,9 @@ def test_parameterized_fixture_caching(self, pytester: Pytester) -> None: def pytest_generate_tests(metafunc): if "my_fixture" in metafunc.fixturenames: - param = "d%s" % "1" - print("param id=%d" % id(param), flush=True) - metafunc.parametrize("my_fixture", [param, "d2"], indirect=True) + # Use unique objects for parametrization (as opposed to small strings + # and small integers which are singletons). + metafunc.parametrize("my_fixture", [[1], [2]], indirect=True) @pytest.fixture(scope='session') def my_fixture(request): From 3153ab2e661994bbf52ca7ec4889d28ac8b44aa6 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 13 Jul 2024 10:52:14 -0300 Subject: [PATCH 4/6] Identity comparsion first --- src/_pytest/fixtures.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index a55ccf3ed51..c4a5c9d72ea 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1057,12 +1057,14 @@ def execute(self, request: SubRequest) -> FixtureValue: if self.cached_result is not None: cache_key = self.cached_result[1] - # Coerce the comparison into a bool (#12600), and if that fails, fall back to an identity check: - # `__eq__` is not required to return a bool, and sometimes doesn't, e.g., numpy arrays (#6497). - try: - cache_hit = bool(my_cache_key == cache_key) - except (ValueError, RuntimeError): - cache_hit = my_cache_key is cache_key + # First attempt to use 'is' for performance reasons (for example numpy arrays (#6497)). + cache_hit = my_cache_key is cache_key + if not cache_hit: + # If they are not the same, fallback to a bool comparison (#12600). + try: + cache_hit = bool(my_cache_key == cache_key) + except (ValueError, RuntimeError): + cache_hit = False if cache_hit: if self.cached_result[2] is not None: From fa7147b1eb49b0b79031b652d10125d57feee669 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 16 Jul 2024 21:13:17 -0300 Subject: [PATCH 5/6] Change comparison order --- src/_pytest/fixtures.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index c4a5c9d72ea..7d0b40b150a 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1053,18 +1053,16 @@ def execute(self, request: SubRequest) -> FixtureValue: requested_fixtures_that_should_finalize_us.append(fixturedef) # Check for (and return) cached value/exception. - my_cache_key = self.cache_key(request) if self.cached_result is not None: + request_cache_key = self.cache_key(request) cache_key = self.cached_result[1] - - # First attempt to use 'is' for performance reasons (for example numpy arrays (#6497)). - cache_hit = my_cache_key is cache_key - if not cache_hit: - # If they are not the same, fallback to a bool comparison (#12600). - try: - cache_hit = bool(my_cache_key == cache_key) - except (ValueError, RuntimeError): - cache_hit = False + try: + # Attempt to make a normal == check: this might fail for objects + # which do not implement the standard comparison (like numpy arrays -- #6497). + cache_hit = bool(request_cache_key == cache_key) + except (ValueError, RuntimeError): + # If the comparison raises, use 'is' as fallback. + cache_hit = request_cache_key is cache_key if cache_hit: if self.cached_result[2] is not None: From a8229c204e4873eff050ebeda81ea03e4d9344c8 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 17 Jul 2024 09:35:28 -0300 Subject: [PATCH 6/6] Update changelog/6962.bugfix.rst Co-authored-by: Ran Benita --- changelog/6962.bugfix.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changelog/6962.bugfix.rst b/changelog/6962.bugfix.rst index 9557d7b173e..030b6e06392 100644 --- a/changelog/6962.bugfix.rst +++ b/changelog/6962.bugfix.rst @@ -1 +1,2 @@ -Fixed bug where parametrized fixtures were not being cached correctly, being recreated every time. +Parametrization parameters are now compared using `==` instead of `is` (`is` is still used as a fallback if the parameter does not support `==`). +This fixes use of parameters such as lists, which have a different `id` but compare equal, causing fixtures to be re-computed instead of being cached.