From af04300e72f1d4b9ed0848296710d9fab704dcde Mon Sep 17 00:00:00 2001 From: Will Shanks Date: Thu, 9 Nov 2023 23:37:54 -0500 Subject: [PATCH] Remove usage of QiskitTestCase (#1285) 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](https://github.com/Qiskit-Extensions/qiskit-experiments/pull/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](https://github.com/Qiskit-Extensions/qiskit-experiments/pull/1282). --- CONTRIBUTING.md | 16 +- requirements-dev.txt | 2 + test/base.py | 433 ++++++++++++++----------- test/framework/test_store_init_args.py | 5 +- test/test_base.py | 27 ++ tox.ini | 5 + 6 files changed, 298 insertions(+), 190 deletions(-) create mode 100644 test/test_base.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e54b267a0b..cf33b8ffa7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -121,8 +121,8 @@ Note that tests will fail automatically if they do not finish execution within 6 #### STDOUT/STDERR and logging capture -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 capture any text written to stdout, stderr, and log messages and add them as attachments to the tests run so output can be associated with the test case it originated from. However, if you run tests with `stestr` outside of these mechanisms, by default the @@ -134,6 +134,18 @@ stdlib unittest runner, a similar result can be accomplished by using the [`--buffer`](https://docs.python.org/3/library/unittest.html#command-line-options) option (e.g. `python -m unittest discover --buffer ./test/python`). +#### Other testing related settings + +The test code defines some environment variables that may occasionally be useful to set: + ++ `TEST_TIMEOUT`: An integer representing the maximum time a test can take + before it is considered a failure. ++ `QE_USE_TESTTOOLS`: Set this variable to `FALSE`, `0`, or `NO` to have the + tests use `unittest.TestCase` as the base class. Otherwise, the default is +`testtools.TestCase` which is an extension of `unittest.TestCase`. In some +situations, a developer may wish to use a workflow that is not compatible with +the `testtools` extensions. + ### Code style The qiskit-experiments repository uses `black` for code formatting and style and diff --git a/requirements-dev.txt b/requirements-dev.txt index f4f802a434..3ba0a3a7ec 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,6 +1,8 @@ qiskit-terra>=0.45.0 black~=22.0 +fixtures stestr +testtools pylint~=3.0.2 astroid~=3.0.1 # Must be kept aligned to what pylint wants jinja2==3.0.3 diff --git a/test/base.py b/test/base.py index dc7384afb9..028b549b7b 100644 --- a/test/base.py +++ b/test/base.py @@ -16,12 +16,13 @@ import os import json import pickle +import unittest import warnings from typing import Any, Callable, Optional import fixtures +import testtools import uncertainties -from qiskit.test import QiskitTestCase from qiskit.utils.deprecation import deprecate_func from qiskit_experiments.framework import ( @@ -34,194 +35,254 @@ # Fail tests that take longer than this TEST_TIMEOUT = int(os.environ.get("TEST_TIMEOUT", 60)) +# 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_TESTTOOLS", "TRUE").lower() not in ("false", "0", "no") -class QiskitExperimentsTestCase(QiskitTestCase): - """Qiskit Experiments specific extra functionality for test cases.""" - - def setUp(self): - super().setUp() - self.useFixture(fixtures.Timeout(TEST_TIMEOUT, gentle=True)) - - @classmethod - def setUpClass(cls): - """Set-up test class.""" - super().setUpClass() - - # Some functionality may be deprecated in Qiskit Experiments. If the deprecation warnings aren't - # filtered, the tests will fail as ``QiskitTestCase`` sets all warnings to be treated as an error - # by default. - # pylint: disable=invalid-name - allow_deprecationwarning_message = [ - # TODO: Remove in 0.6, when submodule `.curve_analysis.visualization` is removed. - r".*Plotting and drawing functionality has been moved", - r".*Legacy drawers from `.curve_analysis.visualization are deprecated", - ] - for msg in allow_deprecationwarning_message: - warnings.filterwarnings("default", category=DeprecationWarning, message=msg) - - def assertExperimentDone( - self, - experiment_data: ExperimentData, - timeout: float = 120, - ): - """Blocking execution of next line until all threads are completed then - checks if status returns Done. - - Args: - experiment_data: Experiment data to evaluate. - timeout: The maximum time in seconds to wait for executor to complete. - """ - experiment_data.block_for_results(timeout=timeout) - - self.assertEqual( - experiment_data.status(), - ExperimentStatus.DONE, - msg="All threads are executed but status is not DONE. " + experiment_data.errors(), +def create_base_test_case(use_testtools: bool) -> unittest.TestCase: + """Create the base test case class for package tests + + This function produces the base class for qiskit-experiments tests using + either ``unittest.TestCase`` or ``testtools.TestCase`` for the base class. + The creation of the class is done in this function rather than directly + executed in the module so that, even when ``USE_TESTTOOLS`` is true, a + ``unittest`` base class can be produced for ``test_base.py`` to check that + no hard-dependence on ``testtools`` has been introduced. + """ + if use_testtools: + + class BaseTestCase(testtools.TestCase): + """Base test class.""" + + # testtools maintains their own version of assert functions which mostly + # behave as value adds to the std unittest assertion methods. However, + # for assertEquals and assertRaises modern unittest has diverged from + # the forks in testtools and offer more (or different) options that are + # incompatible testtools versions. Just use the stdlib versions so that + # our tests work as expected. + assertRaises = unittest.TestCase.assertRaises + assertEqual = unittest.TestCase.assertEqual + + def setUp(self): + super().setUp() + if os.environ.get("QISKIT_TEST_CAPTURE_STREAMS"): + stdout = self.useFixture(fixtures.StringStream("stdout")).stream + self.useFixture(fixtures.MonkeyPatch("sys.stdout", stdout)) + stderr = self.useFixture(fixtures.StringStream("stderr")).stream + self.useFixture(fixtures.MonkeyPatch("sys.stderr", stderr)) + self.useFixture(fixtures.LoggerFixture(nuke_handlers=False, level=None)) + + else: + + class BaseTestCase(unittest.TestCase): + """Base test class.""" + + def useFixture(self, fixture): # pylint: disable=invalid-name + """Shim so that useFixture can be called in subclasses + + useFixture is a testtools.TestCase method. The actual fixture is + not used when using unittest. + """ + + class QETestCase(BaseTestCase): + """Qiskit Experiments specific extra functionality for test cases.""" + + def setUp(self): + super().setUp() + self.useFixture(fixtures.Timeout(TEST_TIMEOUT, gentle=True)) + + @classmethod + def setUpClass(cls): + """Set-up test class.""" + super().setUpClass() + + warnings.filterwarnings("error", category=DeprecationWarning) + + # Some functionality may be deprecated in Qiskit Experiments. If + # the deprecation warnings aren't filtered, the tests will fail as + # ``QiskitTestCase`` sets all warnings to be treated as an error by + # default. + # pylint: disable=invalid-name + allow_deprecationwarning_message = [ + # TODO: Remove in 0.6, when submodule `.curve_analysis.visualization` is removed. + r".*Plotting and drawing functionality has been moved", + r".*Legacy drawers from `.curve_analysis.visualization are deprecated", + ] + for msg in allow_deprecationwarning_message: + warnings.filterwarnings("default", category=DeprecationWarning, message=msg) + + def assertExperimentDone( + self, + experiment_data: ExperimentData, + timeout: Optional[float] = None, + ): + """Blocking execution of next line until all threads are completed then + checks if status returns Done. + + Args: + experiment_data: Experiment data to evaluate. + timeout: The maximum time in seconds to wait for executor to + complete. Defaults to the value of ``TEST_TIMEOUT``. + """ + if timeout is None: + timeout = TEST_TIMEOUT + experiment_data.block_for_results(timeout=timeout) + + self.assertEqual( + experiment_data.status(), + ExperimentStatus.DONE, + msg="All threads are executed but status is not DONE. " + experiment_data.errors(), + ) + + def assertEqualExtended( + self, + first: Any, + second: Any, + *, + msg: Optional[str] = None, + strict_type: bool = False, + ): + """Extended equality assertion which covers Qiskit Experiments classes. + + .. note:: + Some Qiskit Experiments class may intentionally avoid implementing + the equality dunder method, or may be used in some unusual situations. + These are mainly caused by to JSON round trip situation, and some custom classes + doesn't guarantee object equality after round trip. + This assertion function forcibly compares input two objects with + the custom equality checker, which is implemented for unittest purpose. + + Args: + first: First object to compare. + second: Second object to compare. + msg: Optional. Custom error message issued when first and second object are not equal. + strict_type: Set True to enforce type check before comparison. + """ + default_msg = f"{first} != {second}" + + self.assertTrue( + is_equivalent(first, second, strict_type=strict_type), + msg=msg or default_msg, + ) + + def assertRoundTripSerializable( + self, + obj: Any, + *, + check_func: Optional[Callable] = None, + strict_type: bool = False, + ): + """Assert that an object is round trip serializable. + + Args: + obj: the object to be serialized. + check_func: Optional, a custom function ``check_func(a, b) -> bool`` + to check equality of the original object with the decoded + object. If None :meth:`.assertEqualExtended` is called. + strict_type: Set True to enforce type check before comparison. + """ + try: + encoded = json.dumps(obj, cls=ExperimentEncoder) + except TypeError: + self.fail("JSON serialization raised unexpectedly.") + try: + decoded = json.loads(encoded, cls=ExperimentDecoder) + except TypeError: + self.fail("JSON deserialization raised unexpectedly.") + + if check_func is not None: + self.assertTrue(check_func(obj, decoded), msg=f"{obj} != {decoded}") + else: + self.assertEqualExtended(obj, decoded, strict_type=strict_type) + + def assertRoundTripPickle( + self, + obj: Any, + *, + check_func: Optional[Callable] = None, + strict_type: bool = False, + ): + """Assert that an object is round trip serializable using pickle module. + + Args: + obj: the object to be serialized. + check_func: Optional, a custom function ``check_func(a, b) -> bool`` + to check equality of the original object with the decoded + object. If None :meth:`.assertEqualExtended` is called. + strict_type: Set True to enforce type check before comparison. + """ + try: + encoded = pickle.dumps(obj) + except TypeError: + self.fail("pickle raised unexpectedly.") + try: + decoded = pickle.loads(encoded) + except TypeError: + self.fail("pickle deserialization raised unexpectedly.") + + if check_func is not None: + self.assertTrue(check_func(obj, decoded), msg=f"{obj} != {decoded}") + else: + self.assertEqualExtended(obj, decoded, strict_type=strict_type) + + @classmethod + @deprecate_func( + since="0.6", + additional_msg="Use test.extended_equality.is_equivalent instead.", + pending=True, + package_name="qiskit-experiments", ) + def json_equiv(cls, data1, data2) -> bool: + """Check if two experiments are equivalent by comparing their configs""" + return is_equivalent(data1, data2) - def assertEqualExtended( - self, - first: Any, - second: Any, - *, - msg: Optional[str] = None, - strict_type: bool = False, - ): - """Extended equality assertion which covers Qiskit Experiments classes. - - .. note:: - Some Qiskit Experiments class may intentionally avoid implementing - the equality dunder method, or may be used in some unusual situations. - These are mainly caused by to JSON round trip situation, and some custom classes - doesn't guarantee object equality after round trip. - This assertion function forcibly compares input two objects with - the custom equality checker, which is implemented for unittest purpose. - - Args: - first: First object to compare. - second: Second object to compare. - msg: Optional. Custom error message issued when first and second object are not equal. - strict_type: Set True to enforce type check before comparison. - """ - default_msg = f"{first} != {second}" - - self.assertTrue( - is_equivalent(first, second, strict_type=strict_type), - msg=msg or default_msg, + @staticmethod + @deprecate_func( + since="0.6", + additional_msg="Use test.extended_equality.is_equivalent instead.", + pending=True, + package_name="qiskit-experiments", ) + def ufloat_equiv(data1: uncertainties.UFloat, data2: uncertainties.UFloat) -> bool: + """Check if two values with uncertainties are equal. No correlation is considered.""" + return is_equivalent(data1, data2) + + @classmethod + @deprecate_func( + since="0.6", + additional_msg="Use test.extended_equality.is_equivalent instead.", + pending=True, + package_name="qiskit-experiments", + ) + def analysis_result_equiv(cls, result1, result2): + """Test two analysis results are equivalent""" + return is_equivalent(result1, result2) + + @classmethod + @deprecate_func( + since="0.6", + additional_msg="Use test.extended_equality.is_equivalent instead.", + pending=True, + package_name="qiskit-experiments", + ) + def curve_fit_data_equiv(cls, data1, data2): + """Test two curve fit result are equivalent.""" + return is_equivalent(data1, data2) + + @classmethod + @deprecate_func( + since="0.6", + additional_msg="Use test.extended_equality.is_equivalent instead.", + pending=True, + package_name="qiskit-experiments", + ) + def experiment_data_equiv(cls, data1, data2): + """Check two experiment data containers are equivalent""" + return is_equivalent(data1, data2) + + return QETestCase + - def assertRoundTripSerializable( - self, - obj: Any, - *, - check_func: Optional[Callable] = None, - strict_type: bool = False, - ): - """Assert that an object is round trip serializable. - - Args: - obj: the object to be serialized. - check_func: Optional, a custom function ``check_func(a, b) -> bool`` - to check equality of the original object with the decoded - object. If None :meth:`.assertEqualExtended` is called. - strict_type: Set True to enforce type check before comparison. - """ - try: - encoded = json.dumps(obj, cls=ExperimentEncoder) - except TypeError: - self.fail("JSON serialization raised unexpectedly.") - try: - decoded = json.loads(encoded, cls=ExperimentDecoder) - except TypeError: - self.fail("JSON deserialization raised unexpectedly.") - - if check_func is not None: - self.assertTrue(check_func(obj, decoded), msg=f"{obj} != {decoded}") - else: - self.assertEqualExtended(obj, decoded, strict_type=strict_type) - - def assertRoundTripPickle( - self, - obj: Any, - *, - check_func: Optional[Callable] = None, - strict_type: bool = False, - ): - """Assert that an object is round trip serializable using pickle module. - - Args: - obj: the object to be serialized. - check_func: Optional, a custom function ``check_func(a, b) -> bool`` - to check equality of the original object with the decoded - object. If None :meth:`.assertEqualExtended` is called. - strict_type: Set True to enforce type check before comparison. - """ - try: - encoded = pickle.dumps(obj) - except TypeError: - self.fail("pickle raised unexpectedly.") - try: - decoded = pickle.loads(encoded) - except TypeError: - self.fail("pickle deserialization raised unexpectedly.") - - if check_func is not None: - self.assertTrue(check_func(obj, decoded), msg=f"{obj} != {decoded}") - else: - self.assertEqualExtended(obj, decoded, strict_type=strict_type) - - @classmethod - @deprecate_func( - since="0.6", - additional_msg="Use test.extended_equality.is_equivalent instead.", - pending=True, - package_name="qiskit-experiments", - ) - def json_equiv(cls, data1, data2) -> bool: - """Check if two experiments are equivalent by comparing their configs""" - return is_equivalent(data1, data2) - - @staticmethod - @deprecate_func( - since="0.6", - additional_msg="Use test.extended_equality.is_equivalent instead.", - pending=True, - package_name="qiskit-experiments", - ) - def ufloat_equiv(data1: uncertainties.UFloat, data2: uncertainties.UFloat) -> bool: - """Check if two values with uncertainties are equal. No correlation is considered.""" - return is_equivalent(data1, data2) - - @classmethod - @deprecate_func( - since="0.6", - additional_msg="Use test.extended_equality.is_equivalent instead.", - pending=True, - package_name="qiskit-experiments", - ) - def analysis_result_equiv(cls, result1, result2): - """Test two analysis results are equivalent""" - return is_equivalent(result1, result2) - - @classmethod - @deprecate_func( - since="0.6", - additional_msg="Use test.extended_equality.is_equivalent instead.", - pending=True, - package_name="qiskit-experiments", - ) - def curve_fit_data_equiv(cls, data1, data2): - """Test two curve fit result are equivalent.""" - return is_equivalent(data1, data2) - - @classmethod - @deprecate_func( - since="0.6", - additional_msg="Use test.extended_equality.is_equivalent instead.", - pending=True, - package_name="qiskit-experiments", - ) - def experiment_data_equiv(cls, data1, data2): - """Check two experiment data containers are equivalent""" - return is_equivalent(data1, data2) +QiskitExperimentsTestCase = create_base_test_case(USE_TESTTOOLS) diff --git a/test/framework/test_store_init_args.py b/test/framework/test_store_init_args.py index df46c68a8d..87ebf90052 100644 --- a/test/framework/test_store_init_args.py +++ b/test/framework/test_store_init_args.py @@ -12,7 +12,8 @@ """Tests for base experiment framework.""" -from qiskit.test import QiskitTestCase +from test.base import QiskitExperimentsTestCase + from qiskit_experiments.framework.store_init_args import StoreInitArgs @@ -58,7 +59,7 @@ def __init__(self, a, b, c="default_c", d="default_d"): pass -class TestSettings(QiskitTestCase): +class TestSettings(QiskitExperimentsTestCase): """Test Settings mixin""" # pylint: disable = missing-function-docstring diff --git a/test/test_base.py b/test/test_base.py new file mode 100644 index 0000000000..cf5c010c96 --- /dev/null +++ b/test/test_base.py @@ -0,0 +1,27 @@ +# This code is part of Qiskit. +# +# (C) Copyright IBM 2021. +# +# This code is licensed under the Apache License, Version 2.0. You may +# obtain a copy of this license in the LICENSE.txt file in the root directory +# of this source tree or at http://www.apache.org/licenses/LICENSE-2.0. +# +# Any modifications or derivative works of this code must retain this +# copyright notice, and modified files need to carry a notice indicating +# that they have been altered from the originals. +""" +Tests for qiskit-experiments base test module +""" + +from test.base import create_base_test_case + + +UnittestBase = create_base_test_case(use_testtools=False) + + +class TestQiskitExperimentsTestCaseWithUnittest(UnittestBase): + """Test QiskitExperimentsTestCase behavior when not based on testtools.TestCase""" + + def test_test(self): + """Test that a test not based on ``testtools`` can run""" + pass diff --git a/tox.ini b/tox.ini index 6e89cda43a..bc7596d0bc 100644 --- a/tox.ini +++ b/tox.ini @@ -9,6 +9,7 @@ install_command = pip install -c{toxinidir}/constraints.txt -U {opts} {packages} setenv = VIRTUAL_ENV={envdir} QISKIT_SUPPRESS_PACKAGING_WARNINGS=Y + QISKIT_TEST_CAPTURE_STREAMS=1 deps = -r{toxinidir}/requirements-dev.txt -r{toxinidir}/requirements-extras.txt @@ -18,6 +19,7 @@ passenv = RAYON_NUM_THREADS QISKIT_IBM_* TEST_TIMEOUT + QE_USE_TESTTOOLS commands = stestr run {posargs} [testenv:cover] @@ -36,6 +38,7 @@ install_command = pip install -U {opts} {packages} setenv = VIRTUAL_ENV={envdir} QISKIT_SUPPRESS_PACKAGING_WARNINGS=Y + QISKIT_TEST_CAPTURE_STREAMS=1 deps = git+https://github.com/Qiskit/qiskit-terra -r{toxinidir}/requirements-dev.txt @@ -45,6 +48,8 @@ passenv = QISKIT_PARALLEL RAYON_NUM_THREADS QISKIT_IBM_* + TEST_TIMEOUT + QE_USE_TESTTOOLS commands = stestr run {posargs}