BasicQiskitTestCase and FullQiskitTestCase do not enforce call behaviour consistently
#6746
Comments
jakelishman
added a commit
to jakelishman/qiskit-terra
that referenced
this issue
Jul 15, 2021
The previous split was to avoid having testtools as a core testing dependency, even though it was present in all CI tests. This eventually led to more fracturing of the two classes, where various extra functionality was added into FullQiskitTestCase, but not into BasicQiskitTestCase (see Qiskit#6746). This meant downstream consumers of QiskitTestCase sometimes had to care about how Terra's tests were configured (see Qiskit/qiskit-aer#1283), and running simple local tests without setting the environment variable QISKIT_TEST_CAPTURE_STREAMS would run with different configurations. This unifies the two classes, such that QiskitTestCase is now the parent class of FullQiskitTestCase. All non-testtools/non-fixtures related additions are handled in QiskitTestCase, and FullQiskitTestCase adds on the testtools-specific functionality. FullQiskitTestCase is still not made a subclass of testtools.TestClass because the same issues mentioned in Qiskit#3982 are still present in testtools. Whether to capture streams and whether to use the testtools machinery are two separate concerns, with the availability of the former being strictly dependent on the latter. We can bind to the "full" testtools-based class provided testtools is available, and it internally decides whether it should also add the stream-capturing mechanisms. The shared functionality added to the setUp, setUpClass and tearDown methods means that it is important all subclasses always call up the stack with super(). testtools has always enforced this using custom _run_setup() methods, but this is not available to us when it is not being used as the runner. Instead, the parent class is modified after creation, wrapping certain methods with additional checks, and the __init_subclass__ method is used to influence the creation of children, to add similar tests when they are created.
mergify bot
pushed a commit
that referenced
this issue
Aug 3, 2021
* Unify setup and teardown methods of TestCases The previous split was to avoid having testtools as a core testing dependency, even though it was present in all CI tests. This eventually led to more fracturing of the two classes, where various extra functionality was added into FullQiskitTestCase, but not into BasicQiskitTestCase (see #6746). This meant downstream consumers of QiskitTestCase sometimes had to care about how Terra's tests were configured (see Qiskit/qiskit-aer#1283), and running simple local tests without setting the environment variable QISKIT_TEST_CAPTURE_STREAMS would run with different configurations. This unifies the two classes, such that QiskitTestCase is now the parent class of FullQiskitTestCase. All non-testtools/non-fixtures related additions are handled in QiskitTestCase, and FullQiskitTestCase adds on the testtools-specific functionality. FullQiskitTestCase is still not made a subclass of testtools.TestClass because the same issues mentioned in #3982 are still present in testtools. Whether to capture streams and whether to use the testtools machinery are two separate concerns, with the availability of the former being strictly dependent on the latter. We can bind to the "full" testtools-based class provided testtools is available, and it internally decides whether it should also add the stream-capturing mechanisms. The shared functionality added to the setUp, setUpClass and tearDown methods means that it is important all subclasses always call up the stack with super(). testtools has always enforced this using custom _run_setup() methods, but this is not available to us when it is not being used as the runner. Instead, the parent class is modified after creation, wrapping certain methods with additional checks, and the __init_subclass__ method is used to influence the creation of children, to add similar tests when they are created. * Explicitly mark unused arguments The decorator's internal functions take arguments *args and **kwargs, in order to wrap arbitrary functions, but pylint complains about them not being used. The arguments are explicitly meant to be ignored, which in pylint-speak means prepending with an underscore. * Retain the BaseQiskitTestCase class The previous commit removed this base class without issuing any deprecation warnings. That's clearly wrong. Further, we actually want to properly formalise a separation between a completely public test-additions API, for _all_ Qiskit-family packages to use, and additions which are specific to Terra. `assertDictsAreEqual` is an example of the former, but logging and warning control are examples of the latter. Deciding on a complete split is out-of-scope for this PR, but this commit reinstates the previous names, to avoid breaking downstream code. * Convert docstrings to googledoc format
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Information
What is the current behavior?
The main
QiskitTestClassis bound to eitherBasicQiskitTestClassorFullQiskitTestClass, depending on whetherfixturesandtesttoolsare available, and if theQISKIT_TEST_CAPTURE_STREAMSenvironment variable is set. These two classes have differences in enforcement of certain behaviours, meaning that when running local tests without setting the envvar, you may see spurious passes in new tests. If you usetoxto run the tests locally, it should always arrange the setup such thatFullQiskitTestCasegets used (like in the CI), but if you're just doing quick-and-dirty tests withpython -munittestorstestr, you might getBasicQiskitTestCase.In particular, the
FullsetUpandtearDownmethods require subclasses to explicitly callsuper().setUp()(and similar), and theFullclass silences a lot of deprecation warnings whichBasicdoesn't.Steps to reproduce the problem
Test file
mytest.pyNow, assuming
testtoolsis availableWhat is the expected behavior?
Both should throw the same error.
Suggested solutions
Only functionality which entirely depends on the presence of
testtoolsshould be different between the two classes. A lot of extra functionality (deprecation warnings, etc) could be moved intoBaseQiskitTestCaseand shared.The text was updated successfully, but these errors were encountered: