Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix _compat tests #4926

Merged
merged 13 commits into from
Feb 2, 2022
Merged

Fix _compat tests #4926

merged 13 commits into from
Feb 2, 2022

Conversation

dabacon
Copy link
Collaborator

@dabacon dabacon commented Feb 1, 2022

Fixes some of the out of order test breakage described in #4921

These tests are certainly broken, they are supposed to pass in the function not the result of the function.

Not sure I fully see why this interacts badly with test_deprecated_module tests, but once it hits one of these failing the following parameterized tests fail.

@dabacon dabacon requested review from cduck, vtomole and a team as code owners February 1, 2022 23:39
@dabacon dabacon requested a review from maffoo February 1, 2022 23:39
@CirqBot CirqBot added the Size: XS <10 lines changed label Feb 1, 2022
@dabacon
Copy link
Collaborator Author

dabacon commented Feb 1, 2022

Oh wait not quite fixed correctly yet.

@pavoljuhas
Copy link
Collaborator

Not sure I fully see why this interacts badly with test_deprecated_module tests, but once it hits one of these failing the following parameterized tests fail.

The functions were supposed to run in a subprocess so they do their imports in isolation.
The original code executed imports in the main thread affecting import warnings for others.

BTW, I also saw a missing import error, please add

diff --git a/cirq-core/cirq/_compat_test.py b/cirq-core/cirq/_compat_test.py
index 1dd95bb2..28dfb6dd 100644
--- a/cirq-core/cirq/_compat_test.py
+++ b/cirq-core/cirq/_compat_test.py
@@ -798,6 +798,8 @@ def _test_broken_module_2_inner():
 
 
 def _test_broken_module_3_inner():
+    import cirq.testing._compat_test_data
+
     with cirq.testing.assert_deprecated(deadline="v0.20", count=None):
         with pytest.raises(
             DeprecatedModuleImportError,

@CirqBot CirqBot added the size: S 10< lines changed <50 label Feb 1, 2022
@dabacon
Copy link
Collaborator Author

dabacon commented Feb 1, 2022

OK, yeah patched things up. Yeah these tests were no-ops, so nice to fix.

@dabacon
Copy link
Collaborator Author

dabacon commented Feb 2, 2022

Also added an assert to make sure none of the other cases were failing in this manner.

@dabacon
Copy link
Collaborator Author

dabacon commented Feb 2, 2022

Ah now windows tests are failing. Joy.

@pavoljuhas
Copy link
Collaborator

Tests may be failing on Windows because they use spawn rather than fork to start multiprocessing jobs.
I got some test failures on Linux after hardcoding spawn at that line -

ctx = multiprocessing.get_context('spawn' if os.name == 'nt' else 'fork')

This seems to get fixed after explicitly activating Python warnings in the failing tests.
Please check if the following patch would help - 0001-Activate-warnings-for-multiprocessing-spawn-context.patch.txt

diff --git a/cirq-core/cirq/_compat_test.py b/cirq-core/cirq/_compat_test.py
index 28dfb6dd..e6ab64e3 100644
--- a/cirq-core/cirq/_compat_test.py
+++ b/cirq-core/cirq/_compat_test.py
@@ -667,7 +667,6 @@ def _test_deprecated_module_inner(outdated_method, deprecation_messages):
             deadline='v0.20',
             count=len(deprecation_messages),
         ):
-            import warnings

             warnings.simplefilter('always')
             outdated_method()
@@ -785,6 +784,7 @@ def _test_broken_module_1_inner():


 def _test_broken_module_2_inner():
+    warnings.simplefilter('always')
     with cirq.testing.assert_deprecated(deadline="v0.20", count=None):
         with pytest.raises(
             DeprecatedModuleImportError,
@@ -800,6 +800,7 @@ def _test_broken_module_2_inner():
 def _test_broken_module_3_inner():
     import cirq.testing._compat_test_data

+    warnings.simplefilter('always')
     with cirq.testing.assert_deprecated(deadline="v0.20", count=None):
         with pytest.raises(
             DeprecatedModuleImportError,

@dabacon
Copy link
Collaborator Author

dabacon commented Feb 2, 2022

@pavoljuhas thanks for figuring that out. Windows always does spawn, which I guess resets the warning, so I think your fix fixes things.

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 2, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 2, 2022
@CirqBot CirqBot merged commit 8b71890 into quantumlib:master Feb 2, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 2, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Fixes some of the out of order test breakage described in quantumlib#4921 

These tests are certainly broken, they are supposed to pass in the function not the result of the function.

Not sure I fully see why this interacts badly with test_deprecated_module tests, but once it hits one of these failing the following parameterized tests fail.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Fixes some of the out of order test breakage described in quantumlib#4921 

These tests are certainly broken, they are supposed to pass in the function not the result of the function.

Not sure I fully see why this interacts badly with test_deprecated_module tests, but once it hits one of these failing the following parameterized tests fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50 Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants