larryhastings / appeal

Command-line parsing library for Python 3.
Other
122 stars 7 forks source link

Bump actions/setup-python to v4 #13

Closed hugovk closed 9 months ago

hugovk commented 11 months ago

Follow on from #12.

To be able to use the newish allow-prereleases.

Also run the CI for PRs, so we can see the test results before merge.

And add workflow_dispatch, which adds a button to the UI for manual triggers, less useful but occasionally so.

hugovk commented 11 months ago

The coverage job is failing because it requires 100% coverage, but the tests only cover 76%:

Run coverage report -i --fail-under=100
/opt/hostedtoolcache/Python/3.[11](https://github.com/larryhastings/appeal/actions/runs/5693625230/job/15433112057?pr=13#step:10:12).4/x64/lib/python3.11/site-packages/coverage/report_core.py:113: CoverageWarning: Couldn't parse '/home/runner/work/appeal/appeal/-': No source for code: '/home/runner/work/appeal/appeal/-'. (couldnt-parse)
  coverage._warn(msg, slug="couldnt-parse")
Name                          Stmts   Miss  Cover
-------------------------------------------------
appeal/__init__.py             3496    931    73%
appeal/argument_grouping.py     333    153    54%
appeal/text.py                  257     96    63%
tests/test_all.py               884     29    97%
-------------------------------------------------
TOTAL                          4970   [12](https://github.com/larryhastings/appeal/actions/runs/5693625230/job/15433112057?pr=13#step:10:13)09    76%
Coverage failure: total of 76 is less than fail-under=100

I'd suggest dropping that to --fail-under=76, and increase it as more test coverage is added in follow-ups.

larryhastings commented 11 months ago

I appreciate you taking a look at this, and I'm definitely interested in the other bits of your PR.

However, I don't see the benefit of dropping the coverage failure % to 76. I get that that makes the test pass--but now it's a lame test. My goal (obviously) is to get Appeal to 100% coverage. My thought was to leave the test failing until then so because a) that's accurate, b) it will fill me with shame and thus motivate me to fix it.

Are there practical workflow benefits to reducing the coverage failure % to e.g. 76?

hugovk commented 11 months ago

I'll remove it.

Yes, generally I find it much more useful to always have a green CI. If there's anything that fails, it kind of "teaches" you to tolerate failures, and not pay as close attention to the others. "Oh, there's always a red CI check, just ignore that."

Ensuring 76% is perhaps better than not ensuring anything, it means it won't regress and drop further, and can be ratcheted up as more coverage is added.

But your point (b) is an interesting one :)

larryhastings commented 11 months ago

This PR itself has answered the question "why would I want to use 76% instead of 100%?". I want to live with the shame of not hitting 100% of coverage, sure. But I don't want to upset poor GitHub and its workflow. And I certainly don't want potential committers to be confronted with errors and make them think they caused the breakage.

Not sure the best way to handle this. Changing the success to 76% overall seems best. Though if a PR inadvertently lowered it to 75% it would fail, telling the committer "please bring the coverage back up to 76% and resubmit" seems like a weird thing to say.

We could change it to just always pass the coverage test, no matter the %. But that means folks could submit PRs that whittle down my coverage %, which I don't really want. (Although IIRC I've had zero PRs from other people on Appeal that would affect code coverage. So this doesn't seem like a real problem.)

Anyway, sorry to ask you to re-do it, but please either reinstate the 76% test, or just remove the testing for success entirely. I guess I'll leave it up to you--I'm new to the world of CI and getting PRs so I don't claim to have good sensibilities here yet.

hugovk commented 11 months ago

I've re-added --fail-under=76.

Another option is to use something called continue-on-error, which allows an individual job to be experimental and show as failed/red, but doesn't fail the whole workflow. It's sometimes used when adding a pre-release like 3.12 to the matrix - it could be some dependency doesn't support 3.12 yet. But last time I tried, I struggled to get the syntax right...

larryhastings commented 9 months ago

Thanks, Hugo! I talked to Hynek today about his PRs and he said I should just merge yours.