oscarhiggott / PyMatching

PyMatching: A Python/C++ library for decoding quantum error correcting codes with minimum-weight perfect matching.
Apache License 2.0
178 stars 32 forks source link

Improve tests #58

Closed AdamWRichardson closed 1 year ago

AdamWRichardson commented 1 year ago

Simple PR to add features of pytest into the python tests which hopefully help readability, performance and coverage.

Happy to have comments and suggestions! I wasn't convinced by the changes in the cli_test.py module so happy to change that if required

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (29d709a) 100.00% compared to head (927b014) 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #58 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 3 3 Lines 331 331 ========================================= Hits 331 331 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

AdamWRichardson commented 1 year ago

One extra suggestion I have is that we use tox to do the python testing, that way it tests the packaging side as well and can simplify running CI. The last two commits show what this looks like, happy to revert if you disagree though

AdamWRichardson commented 1 year ago

The CI failing is very strange to me - I cannot seem to re-create the windows failure locally and the MacOS failure doesn't tell me anything on GitHub Actions. If you have any ideas let me know

oscarhiggott commented 1 year ago

Hi @AdamWRichardson thank you very much for the contribution! I would be happy to merge your initial refactor of the tests (up to ab96a79). It's not yet clear to me whether or not it's worth using tox. For example, we need to test all the different python versions and platforms anyway when building the wheels with cibuildwheels in the CI. Given these need to stay, what advantages does tox offer?

AdamWRichardson commented 1 year ago

Thank you! The cibuildwheels is essentially doing the same thing as tox. I was going to say one advantage of tox is that you run the tests on the wheels or sdists that you've built but cibuildwheels also does that. Another advantage tox would give in this case is to be able to specify all build dependencies (cmake, ninja, stim) in one place but keeping commit dd855d9 would also achieve the same thing.

One actual advantage I've found of tox is that it's usually really easy to reproduce CI commands locally (ironically I can't find why the path is so long on Windows). It means you can run all checks locally before pushing as well so then the only thing you have to worry about is different python versions and OSs.

Perfectly happy to keep using cibuildwheels if you would prefer. I think it would be good to still apply the commit mentioned above to support PEP517/8 backends (I was going to do that in another PR but makes sense to do it here instead)

oscarhiggott commented 1 year ago

Thanks! I'd be happy to keep the pyproject.toml changes in dd855d9 once the CI passes for it. I'm not yet sold on tox as it's not yet clear to me if it offers enough advantages over what cibuildwheels already does (e.g. will it still build each wheel in parallel on a separate worker? This significantly speeds up the workflow.). So I'd suggest leaving tox out of this PR for now, and potentially opening a separate PR for it if you're confident it would be a significant improvement (but I obviously can't promise I'd merge it until I see it).

oscarhiggott commented 1 year ago

Thanks, looks great! It would be great if you could add type hints for the test arguments and fixture and then I'd be happy to merge.