Run Rust tests in CI #9593
Run Rust tests in CI #9593
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
| - name: "testQPY" | ||
| type: boolean | ||
| default: false |
There was a problem hiding this comment.
We already were setting these values in all places, so this doesn't impact any call sites.
Here, I think it's better to be explicit and make the job author decide what behavior they want.
There was a problem hiding this comment.
Sounds fine to me - I originally added the default, but I don't feel strongly about it.
|
FYI this adds 1.5 minutes to the preliminary test job to compile the code and tests: https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=43725&view=logs&j=e4ea27c2-3a16-5b32-7be8-0bf30fe9e828&t=f8a8e7cc-e264-5f79-94f3-e9633107b79a. Over time, this will increase by however long our tests end up being I'm fine moving the step out of preliminary if you think it's the better call. I'm not sure what % of PRs are Rust. It's also worth considering that most Rust devs seem to have more SE experience so may be more comfortable running |
Pull Request Test Coverage Report for Build 4197632721
💛 - Coveralls |
Eric-Arellano
added
Changelog: None
| - name: "testQPY" | ||
| type: boolean | ||
| default: false |
There was a problem hiding this comment.
Sounds fine to me - I originally added the default, but I don't feel strongly about it.
I don't think it's necessary. We already see that the Rust tests are executing: https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=43762&view=logs&jobId=e9516d74-feb4-5f0b-f40e-55a569c6da43&j=e4ea27c2-3a16-5b32-7be8-0bf30fe9e828&t=04bdc700-3d87-5e50-bfde-5aef62f2a2ae |
There was a problem hiding this comment.
Yeah, that's fair.
I have a vague concern that if we ever try to use PyO3 to call very small components of Qiskit Python code from the Rust side then we'll need to push this test back behind the "setup venv + install qiskit" steps in the workflow, but it's probably not worth worrying about right now, since afaik we've no plans to do that; it doesn't really fit with our model of how we use Rust.
Agreed that we can revisit need be. In the meantime, only having Python call Rust keeps things simpler. Thanks for the review! |
Summary
We now run all Rust tests in the preliminary Linux test job and when pushing to the repo.
Why, when we already have great Python tests of our Rust interface? Because we're restricting ourselves from testing at lower levels of abstraction, like unit tests for helper functions. We will still keep our Python tests! But some of our Rust code is highly complex now, so it's valuable to have the ability to test at a lower abstraction level.
Details and comments
These Rust tests are intended to be fast. If in the future we make more robust ~integration tests, we'll revisit the CI setup.
The tests run in the preliminary stage to give faster feedback to Rust developers. This is a tradeoff: it means non-Rust PRs are slightly slower. If this proves too slow, then we can move the tests to later.
In the future, we also can add conditional logic to only run these tests if the PR has made any changes to Rust. I didn't do that yet because it's tricky to get it 100% right and wasn't worth the engineering effort. But if our tests get slower in the future, we can revisit this.