Skip to content
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

Unify setup and teardown methods of TestCases #6753

Merged
merged 7 commits into from
Aug 3, 2021

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Jul 15, 2021

Summary

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.

Details and comments

Fix #6746

@jakelishman jakelishman requested a review from a team as a code owner July 15, 2021 11:57
@CLAassistant
Copy link

CLAassistant commented Jul 15, 2021

CLA assistant check
All committers have signed the CLA.

@jakelishman jakelishman force-pushed the unify-testcase-classes branch from cac55e4 to a101e8f Compare July 15, 2021 12:01
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.
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.
@jakelishman jakelishman force-pushed the unify-testcase-classes branch from 6e63fc2 to 9ae7c83 Compare July 15, 2021 13:33
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I really like the approach here and the end state is much better than what was there before, thanks for doing this.

I have one inline concern about backwards compat for some of our downstream consumers. Honestly having this stuff in an exported interface is awful and we should deprecate the entire qiskit.test module and move things around (like having test.mock move to qiskit.providers or somewhere else) and everything else just lives in the tests. But we can save that for a follow up PR.

qiskit/test/base.py Show resolved Hide resolved
@jakelishman
Copy link
Member Author

I mixed my two responses to the comments (see #6753 (comment)). To be honest, I don't especially mind exporting some testing utilities from qiskit.test, but it would be good to make a much clearer distinction between what's a utility intended to be shared by all Qiskit-family packages (and consequently is a public API with all the guarantees that implies), and what's specific to testing Terra's features (which we can consider private). But yeah, a different PR for that.

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.
@jakelishman
Copy link
Member Author

Ok @mtreinish I've reinstated the BaseQiskitTestCase and BasicQiskitTestCase names. The former is the base, and should only contain functionality that can be shared across all Qiskit-family packages. The latter is just an alias to QiskitTestCase, which is the lowest possible Terra-specific case. I haven't really attempted to make any real split yet though, since that's probably better handled in a different PR.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the quick update. I'll open an issue about the deprecation of qiskit.test where we can discuss how exactly we want to do that.

@mtreinish mtreinish added automerge Changelog: None Do not include in changelog labels Aug 3, 2021
@mergify mergify bot merged commit e55485a into Qiskit:main Aug 3, 2021
@mtreinish mtreinish mentioned this pull request Aug 3, 2021
mtreinish added a commit to mtreinish/qiskit-aer that referenced this pull request Aug 4, 2021
In Qiskit/qiskit#6753 the base test classes were fixed to assert
that setUp and setUpClass in the base test class are always called. This
is necessary to ensure that all the warning assertions always work.
However this change had the unintended side effect of causing Aer's CI
to fail because it wasn't calling super().setUpClass(). This commit
makes this change to fix that failure. However, because now we're
running enforcement that tests can't raise unhandled
DeprecationWarnings a few spots in the tests were emitting deprecation
warnings. The deprecated syntax is either fixed or if it's testing
something in Aer that's deprecated an assertion on that warning is added
to fix the failure.
chriseclectic pushed a commit to Qiskit/qiskit-aer that referenced this pull request Aug 5, 2021
* Fix test deprecation warnings and call parent's setUpClass

In Qiskit/qiskit#6753 the base test classes were fixed to assert
that setUp and setUpClass in the base test class are always called. This
is necessary to ensure that all the warning assertions always work.
However this change had the unintended side effect of causing Aer's CI
to fail because it wasn't calling super().setUpClass(). This commit
makes this change to fix that failure. However, because now we're
running enforcement that tests can't raise unhandled
DeprecationWarnings a few spots in the tests were emitting deprecation
warnings. The deprecated syntax is either fixed or if it's testing
something in Aer that's deprecated an assertion on that warning is added
to fix the failure.
@jakelishman jakelishman deleted the unify-testcase-classes branch August 11, 2021 12:35
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BasicQiskitTestCase and FullQiskitTestCase do not enforce call behaviour consistently
4 participants