Remove most of qiskit.test
#11445
Merged
Remove most of qiskit.test
#11445
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakelishman
added
type: qa
|
One or more of the the following people are requested to review this:
|
Merged
jakelishman
force-pushed
the
remove-test-decorators
branch
from
0fe2844 to
b6d9fd4
Compare
jakelishman
commented
Dec 20, 2023
Comment on lines
-102
to
-114
| @staticmethod | ||
| def _get_resource_path(filename, path=Path.TEST): | ||
| """Get the absolute path to a resource. | ||
|
|
||
| Args: | ||
| filename (string): filename or relative path to the resource. | ||
| path (Path): path used as relative to the filename. | ||
|
|
||
| Returns: | ||
| str: the absolute path to the resource. | ||
| """ | ||
| return os.path.normpath(os.path.join(path.value, filename)) |
Comment on lines
-283
to
-222
| class _TestOptions(collections.abc.Mapping): | ||
| """Lazy-loading view onto the test options retrieved from the environment.""" | ||
|
|
||
| __slots__ = ("_options",) | ||
|
|
||
| def __init__(self): | ||
| self._options = None | ||
|
|
||
| def _load(self): | ||
| if self._options is None: | ||
| self._options = get_test_options() | ||
|
|
||
| def __getitem__(self, key): | ||
| self._load() | ||
| return self._options[key] | ||
|
|
||
| def __iter__(self): | ||
| self._load() | ||
| return iter(self._options) | ||
|
|
||
| def __len__(self): | ||
| self._load() | ||
| return len(self._options) | ||
|
|
||
|
|
||
| TEST_OPTIONS = _TestOptions() |
There was a problem hiding this comment.
Inlined the one use of this still in use - checking if run_slow is in the QISKIT_TESTS environment variable.
Comment on lines
-22
to
-75
| class BackendTestCase(QiskitTestCase): | ||
| """Test case for backends. | ||
|
|
||
| Implementers of backends are encouraged to subclass and customize this | ||
| TestCase, as it contains a "canonical" series of tests in order to ensure | ||
| the backend functionality matches the specifications. | ||
|
|
||
| Members: | ||
| backend_cls (BaseBackend): backend to be used in this test case. Its | ||
| instantiation can be further customized by overriding the | ||
| ``_get_backend`` function. | ||
| circuit (QuantumCircuit): circuit to be used for the tests. | ||
| """ | ||
|
|
||
| backend_cls = None | ||
| circuit = ReferenceCircuits.bell() | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.backend = self._get_backend() | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls): | ||
| if cls is BackendTestCase: | ||
| raise SkipTest("Skipping base class tests") | ||
| super().setUpClass() | ||
|
|
||
| def _get_backend(self): | ||
| """Return an instance of a Provider.""" | ||
| return self.backend_cls() # pylint: disable=not-callable | ||
|
|
||
| def test_configuration(self): | ||
| """Test backend.configuration().""" | ||
| configuration = self.backend.configuration() | ||
| return configuration | ||
|
|
||
| def test_properties(self): | ||
| """Test backend.properties().""" | ||
| properties = self.backend.properties() | ||
| if self.backend.configuration().simulator: | ||
| self.assertEqual(properties, None) | ||
| return properties | ||
|
|
||
| def test_status(self): | ||
| """Test backend.status().""" | ||
| status = self.backend.status() | ||
| return status | ||
|
|
||
| def test_run_circuit(self): | ||
| """Test running a single circuit.""" | ||
| job = execute(self.circuit, self.backend) | ||
| result = job.result() | ||
| self.assertEqual(result.success, True) | ||
| return result |
There was a problem hiding this comment.
I moved this to be BasicAerBackendTestMixin in test.python.basicaer, which is the only place it's used within Qiskit.
Comment on lines
-38
to
-64
| def setup_test_logging(logger, log_level, filename): | ||
| """Set logging to file and stdout for a logger. | ||
|
|
||
| Args: | ||
| logger (Logger): logger object to be updated. | ||
| log_level (str): logging level. | ||
| filename (str): name of the output file. | ||
| """ | ||
| # Set up formatter. | ||
| log_fmt = f"{logger.name}.%(funcName)s:%(levelname)s:%(asctime)s: %(message)s" | ||
| formatter = logging.Formatter(log_fmt) | ||
|
|
||
| # Set up the file handler. | ||
| file_handler = logging.FileHandler(filename) | ||
| file_handler.setFormatter(formatter) | ||
| logger.addHandler(file_handler) | ||
|
|
||
| if os.getenv("STREAM_LOG"): | ||
| # Set up the stream handler. | ||
| stream_handler = logging.StreamHandler() | ||
| stream_handler.setFormatter(formatter) | ||
| logger.addHandler(stream_handler) | ||
|
|
||
| # Set the logging level from the environment variable, defaulting | ||
| # to INFO if it is not a valid level. | ||
| level = logging._nameToLevel.get(log_level, logging.INFO) | ||
| logger.setLevel(level) |
There was a problem hiding this comment.
Inlined into QiskitTestCase.setUpClass.
Comment on lines
-67
to
-87
| class Case(dict): | ||
| """<no description>""" | ||
|
|
||
| pass | ||
|
|
||
|
|
||
| def generate_cases(docstring, dsc=None, name=None, **kwargs): | ||
| """Combines kwargs in Cartesian product and creates Case with them""" | ||
| ret = [] | ||
| keys = kwargs.keys() | ||
| vals = kwargs.values() | ||
| for values in product(*vals): | ||
| case = Case(zip(keys, values)) | ||
| if docstring is not None: | ||
| setattr(case, "__doc__", docstring.format(**case)) | ||
| if dsc is not None: | ||
| setattr(case, "__doc__", dsc.format(**case)) | ||
| if name is not None: | ||
| setattr(case, "__name__", name.format(**case)) | ||
| ret.append(case) | ||
| return ret |
There was a problem hiding this comment.
Moved to test/__init__.py, which is the only place it was used.
Comment on lines
-20
to
-59
| class ProviderTestCase(QiskitTestCase): | ||
| """Test case for Providers. | ||
|
|
||
| Implementers of providers are encouraged to subclass and customize this | ||
| TestCase, as it contains a "canonical" series of tests in order to ensure | ||
| the provider functionality matches the specifications. | ||
|
|
||
| Members: | ||
| provider_cls (BaseProvider): provider to be used in this test case. Its | ||
| instantiation can be further customized by overriding the | ||
| ``_get_provider`` function. | ||
| backend_name (str): name of a backend provided by the provider. | ||
| """ | ||
|
|
||
| provider_cls = None | ||
| backend_name = "" | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.provider = self._get_provider() | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls): | ||
| if cls is ProviderTestCase: | ||
| raise SkipTest("Skipping base class tests") | ||
| super().setUpClass() | ||
|
|
||
| def _get_provider(self): | ||
| """Return an instance of a Provider.""" | ||
| return self.provider_cls() # pylint: disable=not-callable | ||
|
|
||
| def test_backends(self): | ||
| """Test the provider has backends.""" | ||
| backends = self.provider.backends() | ||
| self.assertTrue(len(backends) > 0) | ||
|
|
||
| def test_get_backend(self): | ||
| """Test getting a backend from the provider.""" | ||
| backend = self.provider.get_backend(name=self.backend_name) | ||
| self.assertEqual(backend.name(), self.backend_name) |
There was a problem hiding this comment.
Inlined into the BasicAer provider test, which was the only user.
Pull Request Test Coverage Report for Build 7502969833
💛 - Coveralls |
This removes large tracts of `qiskit.test` that Qiskit itself is no longer using, or inlines a couple of components that were either exceptionally simple or used only once. A few tests that made heavier use of the removed features have been removed rather than replacing the functionality, since those had not actually been running due to dependencies on IBMQ that have not been fulfilled for a long time. This prepares for a more complete removal of `qiskit.test`, if we choose to, but such a change raises the possibility of rather more merge conflicts (due to the need to touch every test file), so is best done separately.
jakelishman
force-pushed
the
remove-test-decorators
branch
from
b6d9fd4 to
bc3df08
Compare
|
Now rebased over #11513. |
1ucian0
approved these changes
Jan 15, 2024
|
Yeah, it's a nice feeling now until we have to deal with all the little bits and bobs all over the place that things like the IBM Runtime are using and we didn't know about... :(. But at least we've the promise of reduced maintenance burden down the road; getting all this stuff cleaned up improves loads of stuff for us across the board - less to keep updated with changing libraries and Python versions, less to run |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Hopefully, we learn about then soon, as some dependants check against |
Closed
ShellyGarion
pushed a commit
to ShellyGarion/qiskit-terra
that referenced
this pull request
Jan 18, 2024
This removes large tracts of `qiskit.test` that Qiskit itself is no longer using, or inlines a couple of components that were either exceptionally simple or used only once. A few tests that made heavier use of the removed features have been removed rather than replacing the functionality, since those had not actually been running due to dependencies on IBMQ that have not been fulfilled for a long time. This prepares for a more complete removal of `qiskit.test`, if we choose to, but such a change raises the possibility of rather more merge conflicts (due to the need to touch every test file), so is best done separately.
This was referenced Jan 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This removes large tracts of
qiskit.testthat Qiskit itself is no longer using, or inlines a couple of components that were either exceptionally simple or used only once. A few tests that made heavier use of the removed features have been removed rather than replacing the functionality, since those had not actually been running due to dependencies on IBMQ that have not been fulfilled for a long time.This prepares for a more complete removal of
qiskit.test, if we choose to, but such a change raises the possibility of rather more merge conflicts (due to the need to touch every test file), so is best done separately.Details and comments
This is effectively a rework of most of #10998 (which I forgot existed when I started doing this), but separates out the concerns a little bit. This PR severely trims down the content of
qiskit.testwithout moving any of the remaining content - we can do that in a follow-up if we choose to, but we don't technically need to.We can technically leave
qiskit.testin place after its "removal", because it's not documented, so is not part of our public interface. If we do choose to remove it completely, and move its remaining components to thetest"package" (like #10998 does), it's probably better to do that step in a separate PR, since it touches every single test file in the repo, so is at large risk of merge conflicts.Relevant deprecations are in #11001.
You can see what's left in
qiskit.testhere. Everything in there is suitable for moving to thetest.pythonroot, though pragmatically, I think there might be some dynamic-circuits tests in IBM-internal code that use the canonicalisation inqiskit.test._canonical. For the most part, those changes are no longer necessary asQuantumCircuit.__eq__now does most of the canonicalisation on-the-fly during the comparison.