Skip to content

Update unittest with run success check#568

Merged
nkanazawa1989 merged 9 commits into
qiskit-community:mainfrom
nkanazawa1989:upgrade/test_for_experiment_success
Jan 27, 2022
Merged

Update unittest with run success check#568
nkanazawa1989 merged 9 commits into
qiskit-community:mainfrom
nkanazawa1989:upgrade/test_for_experiment_success

Conversation

@nkanazawa1989
Copy link
Copy Markdown
Collaborator

Summary

When you run Experiment.run this will suppress all errors incurred durning the execution and errors will be output as user warning

"Possibly incomplete analysis results: an analysis callback raised an error."

with stored error traceback. This is problematic behavior in unittest since CI might miss some unexpected bugs as maybe reported in #533.

This PR adds assertSuccess and assertFail checks that are expected to be called right after every run method call.

Details and comments

Comment thread test/base.py
Comment thread test/calibration/experiments/test_drag.py Outdated
Comment thread test/base.py Outdated
@nkanazawa1989 nkanazawa1989 added the blocked Can't be worked on until another issue is resolved label Dec 9, 2021
@nkanazawa1989
Copy link
Copy Markdown
Collaborator Author

I blocked this until #573 is resolved.

@nkanazawa1989 nkanazawa1989 removed the blocked Can't be worked on until another issue is resolved label Jan 21, 2022
@nkanazawa1989
Copy link
Copy Markdown
Collaborator Author

Now this PR adds unittest for #599. Calling block_for_results method in unittest is replaced with self.assertComplete which also checks if experiment status becomes Done after execution otherwise raise an error.

@nkanazawa1989 nkanazawa1989 force-pushed the upgrade/test_for_experiment_success branch from be86fad to 801ca30 Compare January 21, 2022 08:52
assertSuccess -> assertComplete
@nkanazawa1989 nkanazawa1989 force-pushed the upgrade/test_for_experiment_success branch from 801ca30 to 97a7415 Compare January 21, 2022 08:54
Copy link
Copy Markdown
Collaborator

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added assert is basically functioning as a way to automatically block for results and check an experiment is Done, so I think the assert name should be changed to reflect that.
It is basically a more opaque that you are just checking:

assertEqual(data.block_for_results().status(), ExperimentStatus.DONE)

Also I think the assertFail test isn't needed and can just be removed, it would be better to be clear in the places where you are looking for errors to assert status = ERROR.

Comment thread test/base.py Outdated
Comment thread test/base.py Outdated
Comment thread test/base.py Outdated
Comment thread test/database_service/test_db_experiment_data.py Outdated
Comment thread test/calibration/experiments/test_rabi.py Outdated
- assertComplete -> assertExperimentDone
- remove assertFail
- update reno
@nkanazawa1989
Copy link
Copy Markdown
Collaborator Author

Thanks Chris. These all make sense. I updated the PR according to your comments.

@nkanazawa1989 nkanazawa1989 reopened this Jan 27, 2022
@nkanazawa1989 nkanazawa1989 merged commit f406d83 into qiskit-community:main Jan 27, 2022
@nkanazawa1989 nkanazawa1989 deleted the upgrade/test_for_experiment_success branch January 27, 2022 04:59
paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022
* add run success check

* fix readout angle test

* update reno

Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants