Pytest one file

[Edoardo-Pedicillo]

This PR has been opened in order to fix #786.
Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[alecandido]

So, --pyargs is a boolean flag, when you pass as a member of addopts list you should put it alone:

addopts = ["--pyargs", "..."]

What it does is changing the way the positional arguments are interpreted, so qibo means "the qibo package", but you can't pass paths any longer.
This is exactly what it was breaking pytest <path>, and the origin of #786.

https://docs.pytest.org/en/7.2.x/example/pythoncollection.html#interpreting-cmdline-arguments-as-python-packages

As a single string, addopts could accept --pyargs qibo ..., because --pyargs was interpreted as a flag, and qibo as a positional argument. The moment you turned into a list, you should have split the two elements, they are not a single option followed by its argument, but two separate arguments: a flag option and a positional.

However, you are already specifying the test path, so --pyargs is not required, and it would break passing paths explicitly.

[alecandido]

I moved back the tests to keep the change minimal, and just fix #786.

If @renatomello can confirm this is solving the issue, I would immediately merge this PR, and move the tests and in a separate one (but you can start the new one even immediately @Edoardo-Pedicillo).

[alecandido]

There is a single test failing
https://github.com/qiboteam/qibo/actions/runs/4201951177/jobs/7289505400#step:6:310
and it's really borderline:

assert 1.1305219110140324e-08 < 1e-08

Since it has been written by @renatomello one week ago, maybe you can help to solve it? It would be appreciated
https://github.com/qiboteam/qibo/blame/96b47b3f7153307a3aa6b1642f9ede53b5b24b56/src/qibo/tests/test_quantum_info_superoperator_transformations.py#L198

[renatomello]

There is a single test failing https://github.com/qiboteam/qibo/actions/runs/4201951177/jobs/7289505400#step:6:310 and it's really borderline:

assert 1.1305219110140324e-08 < 1e-08

Since it has been written by @renatomello one week ago, maybe you can help to solve it? It would be appreciated https://github.com/qiboteam/qibo/blame/96b47b3f7153307a3aa6b1642f9ede53b5b24b56/src/qibo/tests/test_quantum_info_superoperator_transformations.py#L198

This is closed by #797

[renatomello]

Tested and can confirm the correct behavior.

[codecov]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (e22f709) compared to base (4d15903).
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff             @@
##            master      #796     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files           97        47     -50     
  Lines        12890      5706   -7184     
===========================================
- Hits         12890      5706   -7184     

Flag	Coverage Δ
unittests	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/conftest.py
tests/test_backends.py
tests/test_callbacks.py
tests/test_cirq.py
tests/test_derivative.py
tests/test_dill.py
tests/test_gates_abstract.py
tests/test_gates_channels.py
tests/test_gates_density_matrix.py
tests/test_gates_gates.py
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[renatomello]

I moved back the tests to keep the change minimal, and just fix #786.

If @renatomello can confirm this is solving the issue, I would immediately merge this PR, and move the tests and in a separate one (but you can start the new one even immediately @Edoardo-Pedicillo).

Is that the reason coverage decreased to 20%?

[alecandido]

Is that the reason coverage decreased to 20%?

Don't believe so, I'm investigating the issue.
The 20% difference is due to the changes wrt master branch, and you can see them in the "Files changed" tab. I believe before was not skipping some tests, or something like that.

[alecandido]

It is weird, because I can not reproduce the low coverage locally, and the differences with master are really minimal (and that's explicitly the reason why I moved back the tests).

Let's run once more, and then I will have to more carefully inspect the logs.

[renatomello]

It is weird, because I can not reproduce the low coverage locally

I couldn't either

[renatomello]

Related to this issue and to #798, I just cloned the repo and tried to run pytest and it wouldn't work, throwing this error

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov=qibo --cov-report=xml

The only way to make it work was to remove these two flags from addopts.

[alecandido]

The only way to make it work was to remove these two flags from addopts.

Unfortunately, in master pytest-cov (and pytest-env as well) are not proper dependencies, and indeed they are installed explicitly in the workflow.

If you clone the repository, switch to this branch, and install with:

pip install '.[tests]'

(i.e. including the extra tests), then the pytest-cov package will be installed as well, and with the plugin available the flags will be recognized.

[alecandido]

I'm still debugging/developing a new solution in #798.

The solution proposed here (split addopts string, dropping --pyargs and qibo arguments) would fix the original problem, but spoiling the coverage. I'm trying to get both, since it is definitely possible.

[Edoardo-Pedicillo]

Have you installed qibo with the command pip install .[tests,docs] ?
EDIT: I tried and the tests work.

[renatomello]

Have you installed qibo with the command pip install .[tests,docs] ? EDIT: I tried and the tests work.

Yes, I did. Same error.

EDIT: @Edoardo-Pedicillo helped me fix by installing pytest-cov manually.

[alecandido]

Yes, I did. Same error.

I believe you did on a different branch, since pytest-cov dependency is newly introduced in this one.

qibo/setup.py

Line 77 in e5fe94f

"pytest-cov",

Please, try to create a fresh environment while on this branch, and repeat the installation. pytest-cov will be installed by pip install '.[tests]'.

[renatomello]

Yes, I did. Same error.

I believe you did on a different branch, since pytest-cov dependency is newly introduced in this one.

qibo/setup.py

Line 77 in e5fe94f

"pytest-cov",

Please, try to create a fresh environment while on this branch, and repeat the installation. pytest-cov will be installed by pip install '.[tests]'.

I was on master. Fixed now.

[alecandido]

The hypothesis of #798 (comment) is confirmed: the coverage was lowered by overwriting the coverage.xml file by subsequent running of pytest examples/.

It is fixed by adding the --cov-append option in 71939c1

If this is the chosen solution (instead of 2. or 3. in #798 (comment)) this should be propagated to Qibolab and Qibocal, but we don't need any change to the workflows.

[renatomello]

The hypothesis of #798 (comment) is confirmed: the coverage was lowered by overwriting the coverage.xml file by subsequent running of pytest examples/.

It is fixed by adding the --cov-append option in 71939c1

If this is the chosen solution (instead of 2. or 3. in #798 (comment)) this should be propagated to Qibolab and Qibocal, but we don't need any change to the workflows.

I tested locally and this is working now @stavros11, allowing running pytest in just one file without having to modify pyproject.toml and maintaining coverage.

[renatomello]

Do we merge this? @alecandido

[alecandido]

For me, it is fine as it is, you can keep going.

Just a final note: I added the dependency on pytest-cov in setup.py, in order to eventually remove it from the workflow. I considered introducing pytest-env for the same reason, but we are not really using it, in this repo in the CI. Thus, I decided that it is useless and (potentially) harmful (very mildly in this case) to have unused dependencies. So I didn't.
Of course, if it will be useful at any time, it takes nothing to update the list :)

[stavros11]

Looks good to me too, thanks. I also like more having the tests outside the code, it also makes it slightly easier to execute single tests. I only have a small issue when I use this locally, I get a lot of warnings that look like

/home/stavros/anaconda3/envs/qibo/lib/python3.9/site-packages/coverage/report.py:114: CoverageWarning: Couldn't parse '/home/stavros/qiboteam/qibo/src/qibo/tests/conftest.py': No source for code: '/home/stavros/qiboteam/qibo/src/qibo/tests/conftest.py'. (couldnt-parse)

and the same for many other files. Are you also getting this? I have not explored it much, so it may be something with my installation.

If this is the chosen solution (instead of 2. or 3. in #798 (comment)) this should be propagated to Qibolab and Qibocal, but we don't need any change to the workflows.

In principle this is not strictly necessary yet, since we don't have coverage from different test runs in these repositories yet, but it won't hurt either.

Just a final note: I added the dependency on pytest-cov in setup.py, in order to eventually remove it from the workflow.

I agree with this too. In fact you could also probably remove pytest from the dependencies and leave only pytest-cov as this will include pytest (now we have both). Maybe we could do the same with pylint if you think it simplifies the workflow.

[alecandido]

Are you also getting this? I have not explored it much, so it may be something with my installation.

I'm investigating. I believe that might be related to the presence of an outdated coverage.xml, on which it is trying to append the new coverage (so in the old file the tests were still there, and now it is getting no info about those files).
The problem might be solved just deleting coverage.xml and htmlcov/. A more drastic approach, almost equivalent to reclone, it would be to use git clean -fdx to remove all files untracked and ignored (with git clean -fd only those untracked would be removed), and then recreate the env and so on.

In principle this is not strictly necessary yet, since we don't have coverage from different test runs in these repositories yet, but it won't hurt either.

Agreed, but it would change the way it works locally in development. So, on Qibo you would append coverage by default, and Qibolab and Qibocal would overwrite coverage.
It is not strictly required, but I'd follow a single pattern for all the three projects.

I agree with this too. In fact you could also probably remove pytest from the dependencies and leave only pytest-cov as this will include pytest (now we have both). Maybe we could do the same with pylint if you think it simplifies the workflow.

I see your point. However, I believe it is better to keep pytest ourselves, also because in this way we have more freedom to impose restrictions on the version itself.
As a rule of thumb, I keep all the dependencies of directly imported packages, or directly used tools (in this case, we do not import pytest-cov, but we use it).

[renatomello]

Merged with new master. Do we have an update on the warnings @stavros11 mentioned?