-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add a decorator for the tests for checking c++ simulator availability #662
Comments
Hi @diego-plan9, I would like to work on this issue. As you suggested, implementing it along the lines of the existing A string search in |
Wonderful, @srujanm ! Yes, the number of files to be changed (~7) seems about right - we can double check during the review of the PR, as outlined in the contribution guidelines. Looking forward to the upcoming PR, and thanks for tackling this issue - don't hesitate on commenting on this issue or on the PR for more clarifications or anything we can help with. |
Hi @diego-plan9, I am almost done with this decorator implementation. There is just one error I am running into that is independent of the decorator. When I run I am working with
|
Hmm, can you specify if you are running into the error in If it is the first case, please do temporarily ignore it in your local tests, as it should be something to be handled in issue #537 (although that test seems to be passing both for linux and for osx https://travis-ci.org/Qiskit/qiskit-terra/jobs/408399777#L971 at least). If it is the second case and you are only experiencing it in the branch with your changes, we can better take a look at it during the actual PR review, once you create it - this way we can try to reproduce it more easily. |
* Added two functions that check for C++ simulator availability * Replaced repetitive code in test files with these functions
Hey @diego-plan9, I saw the error on the I switched to linux though, and confirmed that both |
* Function is now just the unittest skipIf decorator * It acts on both methods and classes
Fixed by #689 |
…ility (Qiskit#689) * Added decorator to check for C++ simulator availability (Qiskit#662) * Added two functions that check for C++ simulator availability
What is the expected enhancement?
In a number of test files in the
test/python/
folder, there is a check at the top of the file for the existence of the C++ simulator: if it is not present, some tests are skipped. In order to avoid duplication and for consistency, it would be nice to have a decorator intest/python/common.py
that we can reuse!Good first contribution
This would actually be a first good contribution, as the tests are a good stepping stone for looking into the internals of
qiskit-terra
if desired, and at the same time the changes should be quite self-contained indeed. It also includes exploring a bit Python decorators, which are a powerful mechanism and rather fun actually!If intending to tackle this issue, please reply in this issue before starting the implementation, and feel free to discuss and suggest other alternatives! 🎉
Suggested implementation
This is just a proposal, there might be better ways - part of the issue is finding the best way indeed and discuss the solutions!
Look for the blocks in the test files that look similar to:
This is the check that is used for determining if the C++ simulator is available.
Create a new function in
test/python/common.py
, named for examplerequires_cpp_simulator()
. In order to make it a decorator, probably the most convenient implementation is via making use offunctools.wraps()
.A good inspiration is the
slow_test()
function in the same file: the behaviour of your decorator will be pretty similar, raising aunittest.SkipTest
if the c++ simulator is not available, and returning the original function otherwise.In the files where the
try
blocks from 1) where found, remove thetry
blocks and instead append the@requires_cpp_simulator
decorator to the class (if all tests need to be skipped) or to the individual tests that need it:The text was updated successfully, but these errors were encountered: