Bundle tests in qiskit package to allow running from install

[nonhermitian]

What is the expected enhancement?

The tests should be able to be run from the installed qiskit package. This is true for all other numeric Python packages. It will also help for testing older releases.

[mtreinish]

Did you also want to include the tests in the sdist (which is something I advocated for in #268 ) or just updating the imports to use absolute imports so they'll run on an installed package?

[nonhermitian]

I want to be able to do qiskit.test() and have the tests run, so in the package.

[diego-plan9]

Can we regroup after 0.7 before proceeding with the implementation? I think it would be nice to have the tests included in the deliverables, and being able to tackle it after the release would hopefully allow enough time for continuing the discussion hinted by @mtreinish, and at the same time reduce the impact of a potentially big PR at this time (and kind of touches with a test overhaul that might be good for Aer as well).

[nonhermitian]

It is not marked for 0.7.

[JohanNicander]

This would ease the integration of qiskit into more exotic hardware (see the qiskit-on-raspberry project for an example).

Examples of functionality would be (as mentioned above)

python -c 'import qiskit; qiskit.test()'

to run the full test suite, but also

python -c 'import qiskit.circuits; qiskit.circuits.test()'

to test only the circuits submodule, for example.

This should preferably be implemented without changing the current testing functionality or without having to rewrite any of the tests.

This can be done by moving the tests from the "external" (external to the qiskit module) test folder and move them into the module itself. To be consistent the best solution would probably be to let every module and submodule handle their tests. That is split up the existing test directory and distribute it over the modules. To make the tests runnable from python without having to implement the functionality in every module/submodule a top-level tester class can be utilized to let modules register that they are testable and let it handle it. This also allows for easy and consistent migration and behavior for all other qiskit elements. I have made a minimal example of how this would look, which can be found here.

Furthermore, I have completed the test migration on a local copy of qiskit-terra (with surprising ease) and plan to implement the top-level tester class shortly, as proof of concept. This repo can be made available on request.

I would gladly keep going and do the migration if there are no objections. In fact, I will keep going and submit a merge request if I don't hear anything else.

[JohanNicander]

I have also noticed that there are multiple ways of testing. Tox utilizes stestr, make test uses unittest, make pytest uses pytest. This is, in my opinion, a bit too much variety. Also now when implementing the integrated testing a test runner has to be picked, or functionality to change test runner has to be implemented. My vote is to stick to pytest since this is the most widely used. Are there any other opinions?

[mtreinish]

I have also noticed that there are multiple ways of testing. Tox utilizes stestr, make test uses unittest, make pytest uses pytest. This is, in my opinion, a bit too much variety. Also now when implementing the integrated testing a test runner has to be picked, or functionality to change test runner has to be implemented. My vote is to stick to pytest since this is the most widely used. Are there any other opinions?

There really isn't anything wrong with variety everyone out there has their own preferred workflow and tool, and we wanted to facilitate that. As a wider project qiskit has standardized on strict unittest compatibility for the test suites, and we use stestr in CI across all the projects. The strict unittest compatibility is to not preclude the use of any runner. All the test runners out there support running unittest. stestr is used in CI primarily for speed since it's a parallel first strictly unittest compatible test runner. The makefile is there for legacy purposes because for a long time that was the only way to run tests and some people are used to running make test to run unittests. We honestly should change make test target to use stestr to match tox and ci. The runner it uses doesn't matter so much to people as long as make test. I agree pytest is widely used and a very nice runner, and that's why there is an alias for using it in the Makefile. But we don't use it as the default primarily to ensure that the test suite remains unittest compatible to not preclude the use of anyone's preffered runner. Pytest's tight bundling with its own testing library and other differences from stdlib unittest makes it very easy for the test suite to not work with any test runner besides pytest which was a situation we wanted to avoid.

That being said if we are to add a qiskit.test() method then for that we should just use the stdlib unittest runner to avoid adding am additional dependency.

[mtreinish]

This would ease the integration of qiskit into more exotic hardware (see the qiskit-on-raspberry project for an example).

Examples of functionality would be (as mentioned above)

python -c 'import qiskit; qiskit.test()'

to run the full test suite, but also

python -c 'import qiskit.circuits; qiskit.circuits.test()'

to test only the circuits submodule, for example.

This should preferably be implemented without changing the current testing functionality or without having to rewrite any of the tests.

This can be done by moving the tests from the "external" (external to the qiskit module) test folder and move them into the module itself. To be consistent the best solution would probably be to let every module and submodule handle their tests. That is split up the existing test directory and distribute it over the modules. To make the tests runnable from python without having to implement the functionality in every module/submodule a top-level tester class can be utilized to let modules register that they are testable and let it handle it. This also allows for easy and consistent migration and behavior for all other qiskit elements. I have made a minimal example of how this would look, which can be found here.

Furthermore, I have completed the test migration on a local copy of qiskit-terra (with surprising ease) and plan to implement the top-level tester class shortly, as proof of concept. This repo can be made available on request.

I would gladly keep going and do the migration if there are no objections. In fact, I will keep going and submit a merge request if I don't hear anything else.

We had this debate a long time ago see #268 and we decided we did not want per package test modules as part of that. My personal opinion is that having per module tests really clutters up the repo needlessly, it also can complicate test discovery for a runner.

The direction this work was leaning before it stalled was to move test/python to qiskit/tests but keep all the tests in one place then add a test() function to the base qiskit module. The complication here though is that since this issue was opened people have added a qiskit.test module which is part of the external api from qiskit-terra (other qiskit elements use fixtures and base classes from there as well as it being the home of the fake backends). This precludes adding a qiskit.test() method because of the name conflict.

[JohanNicander]

We had this debate a long time ago see #268 and we decided we did not want per package test modules as part of that. My personal opinion is that having per module tests really clutters up the repo needlessly, it also can complicate test discovery for a runner.

The direction this work was leaning before it stalled was to move test/python to qiskit/tests but keep all the tests in one place then add a test() function to the base qiskit module. The complication here though is that since this issue was opened people have added a qiskit.test module which is part of the external api from qiskit-terra (other qiskit elements use fixtures and base classes from there as well as it being the home of the fake backends). This precludes adding a qiskit.test() method because of the name conflict.

Ah! I see your point. It is a shame that the qiskit.test namespace is already occupied since this is the most standard test call for the other big python modules. This might be something that could be deprecated and renamed to say qiskit.testing in the feature? That might be something for another issue.

To solve this now, what if we instead go for the structure that you suggested in #268? That is all the tests are located under qiskit.tests. I agree that this is a less cluttered way of organizing them, even though it gives a less "modular" feel. For running the tests internally we could still go with the tester class functionality but mapping the function call to qiskit.unittest() instead of qiskit.test(). We could also then have the qiskit.performancetest() for example, if we want to separate that.

[mtreinish]

This was discussed among @Qiskit/terra-core and the current feeling is that this results in too much code churn weighed against the benefit of being able to run qiskit.test() since the only way this could work is if we moved test/ into the qiskit namespace. I'm going to close this issue since there isn't a consensus on the direction, please feel free to reopen though if I'm missing something or there's more to discuss.