matrix-org / python-canonicaljson

Canonical JSON
Apache License 2.0
31 stars 15 forks source link

Move CI checks to Github Actions #41

Closed H-Shay closed 3 years ago

H-Shay commented 3 years ago

Fixes #38. Moved CI checks to Github Actions, removed old Travis configs, and reformatted some code that was failing black invocation.

H-Shay commented 3 years ago

One thing I want to note is that the unit test invocations are failing because they are set to fail if coverage is below 100%. The test coverage is currently at 85% due to the fact that either lines 26-30 OR lines 31-35 will be tested depending on which frozendict implementation (c-extension or no c-extension) the user is running, thus preventing us from having 100% coverage. I will see what I can do about that in another PR but I wanted to mention it.

DMRobertson commented 3 years ago

One thing I want to note is that the unit test invocations are failing because they are set to fail if coverage is below 100%. The test coverage is currently at 85% due to the fact that either lines 26-30 OR lines 31-35 will be tested depending on which frozendict implementation (c-extension or no c-extension) the user is running, thus preventing us from having 100% coverage. I will see what I can do about that in another PR but I wanted to mention it.

I think "coverage combine" might be what we want here. Not familiar with the details, but maybe https://github.com/matrix-org/synapse/blob/6fcc3e0bc81b4ed738eee702b0e1d193c052d205/tox.ini#L174-L182 is of interest?

H-Shay commented 3 years ago

A few things: I am wondering if we can exclude the code that is specific to the c-implementation of frozendict from the coverage report. This code path is not touched at all unless the test runner is using the frozendict c-implementation, and the only way to ensure that is to ensure that the test runner (like a Github Actions runner) uses that specific implementation, which seems odd. This is the code that is causing the coverage --fail-under-100 invocation to fail and blowing up the CI.

Second, re: adding python 3.10-this blows up on the nose invocation, and it looks like nose is on it's way to being unmaintained: https://nose.readthedocs.io/en/latest/. In this case does it make sense to switch the dependency to something else, or to hold off on upgrading to python 3.10?

DMRobertson commented 3 years ago

A few things: I am wondering if we can exclude the code that is specific to the c-implementation of frozendict from the coverage report. This code path is not touched at all unless the test runner is using the frozendict c-implementation, and the only way to ensure that is to ensure that the test runner (like a Github Actions runner) uses that specific implementation, which seems odd. This is the code that is causing the coverage --fail-under-100 invocation to fail and blowing up the CI.

It'd be nice to have something that checks this works in both modes. But I'm not sure there's an easy way to explicitly say "please install frozendict with the C extension" and "please install without it". (I couldn't see anything in its setup.py, anyway.) Given that, I think it's not worth the effort to account for both options here.

Suggestions:

Second, re: adding python 3.10-this blows up on the nose invocation, and it looks like nose is on it's way to being unmaintained: https://nose.readthedocs.io/en/latest/. In this case does it make sense to switch the dependency to something else, or to hold off on upgrading to python 3.10?

Ouch---looks like something changed from deprecated to removed in 3.10 I think? That link suggests:

New projects should consider using Nose2, py.test, or just plain unittest/unittest2.

Maybe we can replace nose with python -m unittest? (If so, I'd suggest sanity checking that we're using the right Python version by e.g. calling python --version. I'm guessing that tox and the setup-python action will mean this just works, but... just a small bit of paranoia on my part there.)

If that doesn't work, I'd suggest giving nose2 a try.

H-Shay commented 3 years ago

I switched to nose2, seems to work fine! Hopefully this is good to go.

DMRobertson commented 3 years ago

See https://github.com/H-Shay/python-canonicaljson/actions/runs/1309035566 for an example of this run in action

H-Shay commented 3 years ago

Merged.