-
Notifications
You must be signed in to change notification settings - Fork 989
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
Pytest without cirq.google #3894
Pytest without cirq.google #3894
Conversation
…rom_cphase_compiler
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
did you consider pytest's importskip https://docs.pytest.org/en/latest/skipping.html#skipping-on-a-missing-import-dependency instead of our own decorator? |
That's a great pointer - thanks - I'll look into that. |
…at/Cirq into remove_google_from_cphase_compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit but otherwise this is looking pretty clean. I'll need one more pass to confirm that all of my major concerns are covered.
runs-on: ubuntu-16.04 | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: actions/setup-python@v1 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
python-version: '3.8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sufficient? I guess the assumption here is that a cirq-only python3.8 run + an all-cirq python3.(6|7) run is enough to verify that cirq-only python3.(6|7) should work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly! One more implicit assumption beyond that is that pytest.importorskip
works similarly across all 3 versions and all three OSes.
Final(?) pass is complete - all my concerns have been addressed. After #3928 this will be ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved following merge of #3928
Enables testing without the existence of cirq.google.
Namely:
--cirq-only
arg, that deletes the cirq/google directory and runs pytest - also adds a new CI job just for this reason@cirq.testing.skip_if_module_not_exists(module="cirq.google")
- the examples folder, performance benchmarking are places where this is okay. In other places this is considered technical debt and we'll need to figure out how to remove those instances cleanly (probably by creating the test infra for those features that the modules can call into)Note: this should be merged after #3888 (it is merged into this branch).
Closes #3737.