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

Add support for attachments to test execution #3982

Merged
merged 9 commits into from
Sep 9, 2020

Conversation

mtreinish
Copy link
Member

Summary

By default in CI and with tox locally we run all the tests in parallel.
However, this makes things like printing to stdout, warnings on stderr,
or logging during tests difficult to deal with. This is because we'll
have multiple writers to the output streams simultaneously and no way
to figure out which text goes to which test. Luckily stestr and the
subunit protocl it uses internally has a method for adding attachments
to test results to handle this kind of case. Using this each output
stream is associated with the test which generated it and will get
displayed as such. The only issue with this is that the stdlib unittest
doesn't know how to write attachments to it's result stream. There is a
unittest extension library testtools[1] that provides this functionality
in a strictly compatible manner with unittest, however the current state
of the library needs some work before it can work well with more modern
features of stdlib unittest from python>3.5 so we either have to change
the tests to not use these features or come up with an alternative
solution for test attachments.

This commit ports the necessary result streaming functionality from
testtools into the QiskitTestCase base test class and then sets up the
appropriate fixtures to attach output streams from each test. This works
around the current limitation with testtools (I'm slowly working on
fixing this in testtools) but still provides the functionality. When
testtools is at the point it can be a drop in replacement for unittest
as terra is using it we can drop this code.

As part of this change all the test classes that define a setUp have a
missing super() call added. This framework does not work without setUp
in the base class being defined since that's what is used to setup the
stream capture. Moving forward the base test class actually enforces
that super() is called, otherwise the tests will fail.

Details and comments

[1] https://github.com/testing-cabal/testtools

@mtreinish
Copy link
Member Author

For an example of the attachments output: https://travis-ci.com/github/Qiskit/qiskit-terra/jobs/298739756#L586

@mtreinish mtreinish force-pushed the enable-test-attachments branch from 1209db9 to ef902ff Compare March 24, 2020 17:16
@mtreinish mtreinish changed the title [WIP] Add support for attachments to test execution Add support for attachments to test execution Mar 24, 2020
@mtreinish mtreinish force-pushed the enable-test-attachments branch from ef902ff to 8c39dee Compare March 24, 2020 17:20
@mtreinish mtreinish force-pushed the enable-test-attachments branch from 0e40e9c to f617971 Compare April 1, 2020 13:13
@mtreinish mtreinish requested a review from jaygambetta as a code owner April 22, 2020 14:40
@mtreinish
Copy link
Member Author

This is stuck on python 3.5 because of subTest usage. The result reporting mechanism in python 3.5's unittest module differs from python 3.6+ which causes the new test results handler to fail when it encounters a subtest on python 3.5. I can either look at backporting py3.5 compat code from python 3.6 to the results handling added here, or convert subtests to use ddt which just uses standard test case results.

@1ucian0
Copy link
Member

1ucian0 commented Sep 9, 2020

ping @mtreinish !

python 3.5 is reaching EOL.

By default in CI and with tox locally we run all the tests in parallel.
However, this makes things like printing to stdout, warnings on stderr,
or logging during tests difficult to deal with. This is because we'll
have multiple writers to the output streams simultaneously and no way
to figure out which text goes to which test. Luckily stestr and the
subunit protocl it uses internally has a method for adding attachments
to test results to handle this kind of case. Using this each output
stream is associated with the test which generated it and will get
displayed as such. The only issue with this is that the stdlib unittest
doesn't know how to write attachments to it's result stream. There is a
unittest extension library testtools[1] that provides this functionality
in a strictly compatible manner with unittest, however the current state
of the library needs some work before it can work well with more modern
features of stdlib unittest from python>3.5 so we either have to change
the tests to not use these features or come up with an alternative
solution for test attachments.

This commit ports the necessary result streaming functionality from
testtools into the QiskitTestCase base test class and then sets up the
appropriate fixtures to attach output streams from each test. This works
around the current limitation with testtools (I'm slowly working on
fixing this in testtools) but still provides the functionality. When
testtools is at the point it can be a drop in replacement for unittest
as terra is using it we can drop this code.

As part of this change all the test classes that define a setUp have a
missing super() call added. This framework does not work without setUp
in the base class being defined since that's what is used to setup the
stream capture. Moving forward the base test class actually enforces
that super() is called, otherwise the tests will fail.

[1] https://github.com/testing-cabal/testtools
@mtreinish mtreinish force-pushed the enable-test-attachments branch from fab1af2 to 22ef241 Compare September 9, 2020 19:41
@mtreinish mtreinish requested a review from a team as a code owner September 9, 2020 19:41
@mtreinish
Copy link
Member Author

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Great! This will save us some headaches...

@mergify mergify bot merged commit 01ded73 into Qiskit:master Sep 9, 2020
@mtreinish mtreinish deleted the enable-test-attachments branch September 9, 2020 23:05
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Sep 15, 2020
Since Qiskit#3982 merged we've been using the python libraries fixtures and
testtools to construct a custom test results handler that can handle
attachments for parallel test execution. While this is only used by the
test runner the imports could cause an issue for isntalled copies of
terra trying to use the fake backends. Since the requirements-dev.txt
isn't installed for regular installs if a user were to import the
fake_backends with a plain install this would result in an ImportError
because fixtures and/or testtools wasn't installed. Since these aren't
really requirements for terra, and are only required to run tests this
commit wraps the imports in a try block so no error will be raised
unless it's actually a test run.

At the same time fixtures and testtools
are explicitly added to the requirements-dev.txt list. They are
implicitly installed because they're requirements of stestr. But having
them explicitly in the requirements-dev.txt list makes this dependency
for running tests explicit, especially in cases where a user isn't using
stestr as their test runner.
mergify bot pushed a commit that referenced this pull request Sep 16, 2020
* Fix potential import error from test runner

Since #3982 merged we've been using the python libraries fixtures and
testtools to construct a custom test results handler that can handle
attachments for parallel test execution. While this is only used by the
test runner the imports could cause an issue for isntalled copies of
terra trying to use the fake backends. Since the requirements-dev.txt
isn't installed for regular installs if a user were to import the
fake_backends with a plain install this would result in an ImportError
because fixtures and/or testtools wasn't installed. Since these aren't
really requirements for terra, and are only required to run tests this
commit wraps the imports in a try block so no error will be raised
unless it's actually a test run.

At the same time fixtures and testtools
are explicitly added to the requirements-dev.txt list. They are
implicitly installed because they're requirements of stestr. But having
them explicitly in the requirements-dev.txt list makes this dependency
for running tests explicit, especially in cases where a user isn't using
stestr as their test runner.

* Fix lint
mtreinish added a commit to mtreinish/qiskit-aer that referenced this pull request Sep 16, 2020
This commit makes 2 minor changes to the CI configuration to re-enable
building and running tests with the thrust OMP CPU backend. This was lost
in the migration to GHA and means we're not testing the thrust
simulation methods in CI which is a coverage hole. This fixes that
oversight. At the same time this commit adds the
QISKIT_TEST_CAPTURE_STREAMS env variable. Since Qiskit/qiskit#3982
merged the base test class has had support for capturing STDOUT, STDERR,
and python logging as part of the test runs, which is especially
important for our CI configuration where we run tests in parallel. This
just enables this option so we'll have captured output streams
associated with each test method.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Sep 16, 2020
In Qiskit#5071 we tried to fix an implicit testing requirement dependency we
added in Qiskit#3982 for capturing output streams from test runs. However,
that change didn't go far enough in making the capture features opt-in.
While it fixed the hard failure it still made the installation of
testtools and fixtures hard requirement for running tests, even if the
stream capturing was never used. For an example of this see Qiskit#5078. This
commit attempts to remedy this situation by making the stream capturing
opportunistically opt-in. It splits out the common functionality between
the old test class without stream capturing into a new common base
class. Then it adds the stream capturing base class on top of that. The
stream capturing class is only used if testtools and fixtures is
installed and QISKIT_CAPTURE_STREAMS is not set.
mergify bot pushed a commit that referenced this pull request Sep 17, 2020
* Make stream capturing test class opportunistic

In #5071 we tried to fix an implicit testing requirement dependency we
added in #3982 for capturing output streams from test runs. However,
that change didn't go far enough in making the capture features opt-in.
While it fixed the hard failure it still made the installation of
testtools and fixtures hard requirement for running tests, even if the
stream capturing was never used. For an example of this see #5078. This
commit attempts to remedy this situation by making the stream capturing
opportunistically opt-in. It splits out the common functionality between
the old test class without stream capturing into a new common base
class. Then it adds the stream capturing base class on top of that. The
stream capturing class is only used if testtools and fixtures is
installed and QISKIT_CAPTURE_STREAMS is not set.

* Update qiskit/test/base.py

* Update qiskit/test/base.py

* Fix lint

This fixes some small oversights that were breaking the legacy
non-captured stream path that pylint was catching.

Co-authored-by: Luciano Bello <[email protected]>
chriseclectic pushed a commit to Qiskit/qiskit-aer that referenced this pull request Sep 18, 2020
This commit makes 2 minor changes to the CI configuration to re-enable
building and running tests with the thrust OMP CPU backend. This was lost
in the migration to GHA and means we're not testing the thrust
simulation methods in CI which is a coverage hole. This fixes that
oversight. At the same time this commit adds the
QISKIT_TEST_CAPTURE_STREAMS env variable. Since Qiskit/qiskit#3982
merged the base test class has had support for capturing STDOUT, STDERR,
and python logging as part of the test runs, which is especially
important for our CI configuration where we run tests in parallel. This
just enables this option so we'll have captured output streams
associated with each test method.
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request 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 pull request 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
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Nov 24, 2021
In Qiskit#3982 we forked a bunch of internals around results handling and
attachments from the testtools library to enable better stdout, stderr,
and pylogging handling when running with stestr (which is our default
runner in CI). This was necessary at the time because testtools relied
on unittest2 (which was a rolling backport package from unittest to
bring new features from newer python's unittest to older python
versions) that has gone unmaintained for years and was causing
incompatibilities with stdlib unittest on newer python versions. However
as of the testtools 2.5.0 release it no longer depends on unittest2 and
instead inherits directly from unittest fixing the compatibility issues
we were facing previously. This means our fork of the result and
attachment handling from testtools is no longer necessary and we can
just use their TestCase. However, since we do not want to require the
use of testtools outside of running with stestr this commit retains the
same spit logic around when we use it to ensure that we're only
opportunistically using the library if it's installed. This enables
people to no have testtools (which honestly isn't the best maintained
library anymore) taint their code path unless it's already installed.

Fixes Qiskit#7307
mergify bot pushed a commit that referenced this pull request Nov 24, 2021
In #3982 we forked a bunch of internals around results handling and
attachments from the testtools library to enable better stdout, stderr,
and pylogging handling when running with stestr (which is our default
runner in CI). This was necessary at the time because testtools relied
on unittest2 (which was a rolling backport package from unittest to
bring new features from newer python's unittest to older python
versions) that has gone unmaintained for years and was causing
incompatibilities with stdlib unittest on newer python versions. However
as of the testtools 2.5.0 release it no longer depends on unittest2 and
instead inherits directly from unittest fixing the compatibility issues
we were facing previously. This means our fork of the result and
attachment handling from testtools is no longer necessary and we can
just use their TestCase. However, since we do not want to require the
use of testtools outside of running with stestr this commit retains the
same spit logic around when we use it to ensure that we're only
opportunistically using the library if it's installed. This enables
people to no have testtools (which honestly isn't the best maintained
library anymore) taint their code path unless it's already installed.

Fixes #7307
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