Avoid running CI twice on PRs. #461
Conversation
Codecov Report
@@ Coverage Diff @@
## main #461 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 1721 1721
Branches 208 208
=========================================
Hits 1721 1721 |
|
I'm strongly against this as it impedes contribution by making it harder for contributors to fully test their code before creating a PR. I wrote more here: python/typeshed#7066 (comment) I've not used this GitHub Action yet, perhaps it can help? https://github.com/marketplace/actions/skip-duplicate-actions |
|
If I understand correctly, you are saying that by limiting the Personally I either test locally before pushing and/or I rely on the checks that are run on the PR once I create it (which is generally immediately after I pushed the branch on my fork) -- I never looked at the test results on my fork. If this is a use case that we want to support for this repo, we could look into the action you suggested, but I'm not familiar with it. |
Exactly.
Testing locally is good, but CIs are really powerful and allow us to easily test on multiple Python versions. It can be difficult to set up several locally, and keep them all current. This can be a common cause of failures: maybe some new feature is used that's not available on older Pythons, or something is used that's removed in newer. (Not relevant here, but often projects will test on multiple operating systems. This can be even harder to fully test locally, and also a common cause of failures: often something passes on the developer's machine, but not on Windows.) It also helps avoid "works on my machine". By testing on the CI, we know we have the same setup (not always 100% true with CIs, but more so than dev machines :) We know dependencies are defined properly and installed in a clean env. More generally, sometimes testing can take a long time locally, but CIs are usually much faster, especially with a comprehensive test matrix. (On some projects, I've not been able to test locally because it needed to install multiple GB worth of Docker stuff than I had available, but could contribute and test changes via CI.) I believe the use of CIs are of huge benefit for software quality and development, especially when so easy nowadays; Travis CI really revolutionised this, remember how hard testing was before. We should rather encourage people to test on their own forks before creating PRs. Fail fast: shorten the time to finding a failure, enable quicker loops of incremental development. |
While it is accurate to say that it won't trigger automatically on pushes, it has the Here's a screen capture showing how to access the feature on GitHub. |
|
Sure, that's also possible, but it's still an extra impediment. Making the human do more work so the computer can do less. In your screen capture I see some 9 mouse clicks taking about 25 seconds! And then repeat for each push. The tests take around 25s, so could have finished by then. And these tests are so quick, how much of a problem is it really for them to be run twice? (For example, see also python/peps#2215 (comment)) |
|
It's not 9 clicks -- to be fair, I did accidentally click on a branch that I didn't intend to, which cuts it down to 7. If you anticipate running lots of tests, you can always keep the The |
And that's an argument to keep them automated, humans can and will accidentally click the wrong thing :) |
|
True. I'd just noticed this from an email notification on #463 (comment) and thought that the arguments against #461 sounded like they are all addressed by adding a workflow Anyway, I think we are probably bikeshedding at this point given the time scales involved, small build matrix size, and the frequency of changes to this repository. As far as the change proposed in this PR goes from the perspective of a potential contributor to bedevere, I don't have a preference one way or the other for every commit triggering CI builds. |
|
For reference, we discussed this on Discord, and this is a summary of our findings and the options we considered:
It seems that the options are:
Using the "skip duplicate actions" seems a bit overkill to me, and using the Footnotes
|
It uses both the workflow file and checks out the code from the selected branch -- here's the line from the job run in the screen capture showing the branch being checked out https://github.com/nightlark/bedevere/runs/6657800671?check_suite_focus=true#step:2:133 |
|
FWIW, seems like either merging, moving or removing the upstream branch here is the only blocker to fully implementing python/core-workflow#460 on this last repo. |
|
Of the options in #461 (comment):
This is my preference.
My second choice.
My third choice. |
|
FWIW I generally prefer the setup @ezio-melotti is proposing here for my own projects. But I agree with @hugovk that, given the relative infrequency of PRs to the bedevere repo, there isn't really a significant problem being solved here. Given Hugo's strong objection, let's say that the status quo wins this one :) |
|
@CAM-Gerlach Done. |
I noticed that tests are run twice on each PR:
This change avoids that by only running them once whenever a PR is created/updated, and then once it is merged with master.