Remove usage of QiskitTestCase #1285

wshanks · 2023-10-14T03:46:42Z

This change removes the dependence on QiskitTestCase, replacing it
with a direct dependence on unittest.TestCase and
testtools.TestCase.

As with QiskitTestCase, the ability to run the tests based either on
unittest.TestCase or testtools.TestCase (a unittest.TestCase
subclass) is preserved. For qiskit-experiments, the ability is actually
restored because the timeout feature added in
#1246
had introduced a hard dependence on testtools.

Specific changes:

Add testtools and fixtures to requirements-dev.txt as required
test dependencies.
Use QE_USE_TESTTOOLS environment variable to control whether tests
are based on testtools.TestCase rather than checking if testtools
is installed.
Remove some checks for test writing best practices. QiskitTestCase
used extra code to ensure that setUp and other test class methods
always called their parents and that those methods are not called from
individual tests. testtools.TestCase does these checks as well. Since
qiskit-experiments always uses testtools in CI, it can rely on
testtools for these checks and just not do them for the alternate
unittest execution.
Generate QiskitExperimentsTestCase from a create_base_test_case
function. This function allows the base test class to be generated
based on either testtools.TestCase or unittest.TestCase so that the
unittest variant can be tested for regressions even when the
testtools variant is enabled.

Closes #1282.

wshanks · 2023-10-14T03:55:27Z

I think this will keep the parts of QiskitTestCase that we need, but I haven't thought carefully about the details. When I ran the tests locally, they all passed except for 3 that timed out (:disappointed:). I was testing against terra main and made an update for deprecation of qiskit.extensions.

In the current form, I didn't copy over the code from qiskit.test that forces test subclasses to call setUp, tearDown, etc. We could copy it...it is a fair amount of code if we copy all of it (the parts in qiskit.utils are probably similar to qiskit.test in that they are not meant to be used by other packages). We could also just try catch this kind of error in code review.

wshanks · 2023-10-14T13:04:09Z

test/base.py


    @classmethod
    def setUpClass(cls):
        """Set-up test class."""
        super().setUpClass()

+        warnings.filterwarnings("error", category=DeprecationWarning)


QiskitTestCase hard codes many exceptions to this rule. One thing I wanted to see by setting up this PR and testing against terra main was if we could pass without that list of exceptions because it would be hard for us to keep with special deprecation warnings in terra that should be allowed, but thankfully our tests pass without any exceptions.

wshanks · 2023-10-18T20:37:25Z

test/base.py

+# Use testtools by default as a (mostly) drop in replacement for
+# unittest's TestCase. This will enable the fixtures used for capturing stdout
+# stderr, and pylogging to attach the output to stestr's result stream.
+USE_TESTTOOLS = os.environ.get("QE_USE_TESTOOLS", "TRUE") == "TRUE"


I made testtools a required import. It is a dependency of fixtures any way. Still, I personally had a niche use case for the tests not deriving from testtools.TestCase so I left that a possibility. If it becomes a problem we can remove it. My niche case is that for some things I like to use pytest plugins for debugging. pytest can run unittest tests but it can not run testtools tests.

test/base.py

This change removes the dependence on `QiskitTestCase`, replacing it with a direct dependence on `unittest.TestCase` and `testtools.TestCase`. As with `QiskitTestCase`, the ability to run the tests based either on `unittest.TestCase` or `testtools.TestCase` (a `unittest.TestCase` subclass) is preserved. For qiskit-experiments, the ability is actually restored because the timeout feature added in [qiskit-community#1246](qiskit-community#1246) had introduced a hard dependence on `testtools`. Specific changes: * Add `testtools` and `fixtures` to `requirements-dev.txt` as required test dependencies. * Use `QE_USE_TESTTOOLS` environment variable to control whether tests are based on `testtools.TestCase` rather than checking if `testtools` is installed. * Remove some checks for test writing best practices. `QiskitTestCase` used extra code to ensure that `setUp` and other test class methods always called their parents and that those methods are not called from individual tests. `testtools.TestCase` does these checks as well. Since qiskit-experiments always uses `testtools` in CI, it can rely on `testtools` for these checks and just not do them for the alternate `unittest` execution. * Generate `QiskitExperimentsTestCase` from a `create_base_test_case` function. This function allows the base test class to be generated based on either `testtools.TestCase` or `unittest.TestCase` so that the `unittest` variant can be tested for regressions even when the `testtools` variant is enabled.

wshanks · 2023-11-02T17:24:32Z

This should be ready now. I reworked it some after further investigation and updated the pull request description.

It could be backported but it's not really needed until qiskit 1.0. Maybe we will not be supporting the 0.5 branch by then any way?

coruscating · 2023-11-08T05:16:44Z

test/base.py

+        def assertExperimentDone(
+            self,
+            experiment_data: ExperimentData,
+            timeout: float = 120,


Maybe this could be tied to TEST_TIMEOUT? At least it doesn't make sense for the default to be 120 if TEST_TIMEOUT is 60.

Hmm, tying this to TEST_TIMEOUT seems kind of reasonable. In practice that will never be reached unless there is some timing uncertainty in the signal.alarm callback, but it would work for the case when not using testtools. Another option might be to get rid of this timeout and just rely on testtools time out. I think this may have been an earlier attempt to do what the time out fixture is doing now. What do you think?

Also, this time out is not really a change in this PR -- this is existing code that just got shifted down in scope.

Yeah, I only noticed it when reviewing this PR (and I also just noticed a few cases in tests where a large timeout value is being manually passed, which is also effectively not doing anything). I think the parameter could be worth keeping in case someone wants a timeout while not using testtools, and setting it to TEST_TIMEOUT may be a bit easier to use. I'm not very hung up over it either way.

coruscating

Thanks, this looks good to me overall. I didn't realize the testtools switch wasn't working. Should QE_USE_TESTTOOLS be added to passenv in tox.ini so users can use the option with tox? Though I'm not sure why users would turn it off (to disable the timeout maybe?). Maybe this option can be mentioned in the contributor guide too.

wshanks · 2023-11-08T17:58:40Z

Thanks, this looks good to me overall. I didn't realize the testtools switch wasn't working. Should QE_USE_TESTTOOLS be added to passenv in tox.ini so users can use the option with tox? Though I'm not sure why users would turn it off (to disable the timeout maybe?). Maybe this option can be mentioned in the contributor guide too.

I referenced this in my inline comment here. Personally, I have a niche use case for not using testtools. I wasn't sure if it was of wide enough interest to document, but I wanted to keep it possible if it was not too much burden.

My particular use case is that sometimes I prefer to run the tests with pytest instead of stestr to use pytest plugins for debugging. pytest is not fully compatible with testtools, a fact which I established here.

I could add QE_USE_TESTTOOLS to the guide. Currently, we don't even mention TEST_TIMEOUT. I don't know what's getting to be too specific of a use case and could be distracting to a new contributor -- maybe people interested enough in changing the test base class would look at the test code and see the QE_USE_TESTTOOLS option.

You can disable the time out by setting TEST_TIMEOUT=0. I guess you could also set a large number. You are right that we could add the variable to passenv. I am usually running outside of tox when I want to debug inside of a test but it would avoid surprise to pass the variable.

By the way, reading the contributor guide now, I realized that the part about capturing streams does not apply to us because we have been using QiskitTestCase and the stream capture stuff is in FullQiskitTestCase. -- I just checked the Qiskit code again and realized it reassigns QiskitTestCase at the bottom when testtools and fixtures are available. I can add that to the testtools variant here.

I think for this PR I will:

Add mentions of TEST_TIMEOUT and QE_USE_TESTTOOLS to the contributor guide (but not suggest too much about workflows for the latter option because I don't want to distract too much from the main workflow for new contributors reading the guide).
~~Remove the test about capturing streams from the guide~~ Add the stream capturing set up to the testtools class.
Add QE_USE_TESTTOOLS to passenv.

Let me know if you think I should do something different.

mtreinish · 2023-11-08T18:38:53Z

@wshanks fwiw, compatibility with pytest is one of the more frequently asked for features in stestr. I have a wip pr up adding it here: mtreinish/stestr#355 . Longer term we might want to look at using pytest for test suites because I'm not really happy with the state of the testtools stack either.

coruscating · 2023-11-08T19:05:49Z

@wshanks Right, I should have clarified I meant specifically what new users following the contributor guide might want to do with pytest. Right now the contributor guide doesn't detail the testing process at all besides showing how to use some basic options in tox, so we if we want to continue offering the same level of information, we don't necessarily need to document the newer flags. I'm fine with your proposed changes too—I wouldn't spend a long time writing a detailed testing guide.

wshanks · 2023-11-08T22:28:27Z

@mtreinish That is good to know about. So that would allow writing pytest-style tests and executing them with stestr. The main use case I had was running tests with pytest --pdb which starts the debugger on the assertion error in the test. I saw a comment from you that stestr --pdb didn't work well and the alternative was to use python -m testtools.run, but I could not get either of those to recreate the behavior of opening the debugger on the test failure. I could use python -m testtools.run along with import pdb; pdb.set_trace() to enter the debugger before the test failure, but that gets a little tedious compared to opening on the failure automatically.

mtreinish · 2023-11-08T22:48:27Z

Yeah, the stestr PR enables using stestr to run pytest-style tests. There isn't an alternative with stestr to support what pytest --pdb does currently. I think after we support running with pytest we probably could add an option to use pdb like that when in pytest-mode.

The issue with stestr run --pdb and testools.run is exactly what you encountered, none of them will automatically drop into a debugger on failure. It still needs to be done manually because the runners all inherit from unittest.run which doesn't have that feature afaik. What stestr run --pdb does currently is avoid using subprocesses to run tests. By default stestr will launch a new process to run any tests, and with pdb that causes issues if you set trace points in a child process. Using --pdb just tells stestr to run a test id directly in the main process so traces don't get dropped.

coruscating

Looks good besides one minor thing.

coruscating · 2023-11-09T20:42:55Z

CONTRIBUTING.md

-When running tests in parallel using `stestr` either via tox, the Makefile (`make
-test_ci`), or in CI, we set the env variable `QISKIT_TEST_CAPTURE_STREAMS`, which will
+When running tests in parallel using `stestr` either via tox
+or in CI, we set the env variable `QISKIT_TEST_CAPTURE_STREAMS`, which will


This also needs to be added to passenv, right?

Actually, the text says that we are setting it, so I added it to setenv. This matches what is in the Qiskit tox.ini. We have gone the whole time without it. Funny, I haven't really noticed the difference enough to be conscious of it. Maybe the experiments test runs are a little spammier than the Qiskit ones...

wshanks · 2023-11-29T18:01:42Z

@mtreinish Since I just ran into it again, I will mention it here. stestr run --pdb does not work for me because I run into mtreinish/stestr#288 which you opened. That is what I was referring to when I said above "I saw a comment from you that stestr --pdb didn't work well."

This change removes the dependence on `QiskitTestCase`, replacing it with a direct dependence on `unittest.TestCase` and `testtools.TestCase`. As with `QiskitTestCase`, the ability to run the tests based either on `unittest.TestCase` or `testtools.TestCase` (a `unittest.TestCase` subclass) is preserved. For qiskit-experiments, the ability is actually restored because the timeout feature added in [qiskit-community#1246](qiskit-community#1246) had introduced a hard dependence on `testtools`. Specific changes: * Add `testtools` and `fixtures` to `requirements-dev.txt` as required test dependencies. * Use `QE_USE_TESTTOOLS` environment variable to control whether tests are based on `testtools.TestCase` rather than checking if `testtools` is installed. * Remove some checks for test writing best practices. `QiskitTestCase` used extra code to ensure that `setUp` and other test class methods always called their parents and that those methods are not called from individual tests. `testtools.TestCase` does these checks as well. Since qiskit-experiments always uses `testtools` in CI, it can rely on `testtools` for these checks and just not do them for the alternate `unittest` execution. * Generate `QiskitExperimentsTestCase` from a `create_base_test_case` function. This function allows the base test class to be generated based on either `testtools.TestCase` or `unittest.TestCase` so that the `unittest` variant can be tested for regressions even when the `testtools` variant is enabled. Closes [qiskit-community#1282](qiskit-community#1282).

wshanks force-pushed the basetest branch from 3298f18 to d1a4dc6 Compare October 14, 2023 03:55

wshanks commented Oct 14, 2023

View reviewed changes

wshanks mentioned this pull request Oct 16, 2023

Update HamiltonianGate path and add tolerance to PulseBackend #1280

Merged

wshanks commented Oct 18, 2023

View reviewed changes

test/base.py Outdated Show resolved Hide resolved

This was referenced Nov 1, 2023

Increase timeout for Mac CI #1309

Merged

Update pylint to version 3.0 #1310

Merged

wshanks force-pushed the basetest branch from 91db71e to 7a81d8e Compare November 2, 2023 17:18

wshanks marked this pull request as ready for review November 2, 2023 17:21

wshanks requested a review from coruscating November 2, 2023 17:21

wshanks added the Changelog: None Do not include in changelog label Nov 2, 2023

black

5c2b50b

coruscating reviewed Nov 8, 2023

View reviewed changes

wshanks added 5 commits November 8, 2023 16:51

Add QISKIT_CAPTURE_OUTPUT_STREAMS behavior

263d4eb

Use TEST_TIMEOUT as the default for assertExperimentDone

dd7e165

Add QE_USE_TESTTOOLS to tox.ini passenv

e1eb8df

Add description of test variables to contributing guide

e6643cf

Merge remote-tracking branch 'upstream/main' into basetest

204fa64

wshanks requested a review from coruscating November 8, 2023 22:28

coruscating reviewed Nov 9, 2023

View reviewed changes

Add QISKIT_TEST_CAPTURE_STREAMS to passenv

3e4121b

Move QISKIT_TEST_CAPTURE_STREAMS to setenv

46eaf16

coruscating approved these changes Nov 10, 2023

View reviewed changes

wshanks added this pull request to the merge queue Nov 10, 2023

Merged via the queue into qiskit-community:main with commit af04300 Nov 10, 2023
10 checks passed

wshanks deleted the basetest branch November 10, 2023 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of QiskitTestCase #1285

Remove usage of QiskitTestCase #1285

wshanks commented Oct 14, 2023 •

edited

Loading

wshanks commented Oct 14, 2023

wshanks Oct 14, 2023 •

edited

Loading

wshanks Oct 18, 2023

wshanks commented Nov 2, 2023

coruscating Nov 8, 2023 •

edited

Loading

wshanks Nov 8, 2023

coruscating Nov 8, 2023

coruscating left a comment

wshanks commented Nov 8, 2023 •

edited

Loading

mtreinish commented Nov 8, 2023

coruscating commented Nov 8, 2023

wshanks commented Nov 8, 2023

mtreinish commented Nov 8, 2023

coruscating left a comment

coruscating Nov 9, 2023

wshanks Nov 9, 2023

wshanks commented Nov 29, 2023

Remove usage of QiskitTestCase #1285

Remove usage of QiskitTestCase #1285

Conversation

wshanks commented Oct 14, 2023 • edited Loading

wshanks commented Oct 14, 2023

wshanks Oct 14, 2023 • edited Loading

Choose a reason for hiding this comment

wshanks Oct 18, 2023

Choose a reason for hiding this comment

wshanks commented Nov 2, 2023

coruscating Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

wshanks Nov 8, 2023

Choose a reason for hiding this comment

coruscating Nov 8, 2023

Choose a reason for hiding this comment

coruscating left a comment

Choose a reason for hiding this comment

wshanks commented Nov 8, 2023 • edited Loading

mtreinish commented Nov 8, 2023

coruscating commented Nov 8, 2023

wshanks commented Nov 8, 2023

mtreinish commented Nov 8, 2023

coruscating left a comment

Choose a reason for hiding this comment

coruscating Nov 9, 2023

Choose a reason for hiding this comment

wshanks Nov 9, 2023

Choose a reason for hiding this comment

wshanks commented Nov 29, 2023

wshanks commented Oct 14, 2023 •

edited

Loading

wshanks Oct 14, 2023 •

edited

Loading

coruscating Nov 8, 2023 •

edited

Loading

wshanks commented Nov 8, 2023 •

edited

Loading