Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keep running tests even if earlier ones failed #10361

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

geekosaur
Copy link
Collaborator

Matt Pickering says we should always run all tests. This uses continue-on-error: true to run them as long as the build succeeded.

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@ffaf1
Copy link
Collaborator

ffaf1 commented Sep 16, 2024

Aren't we going to saturate CI resources pretty fast this way?

@geekosaur
Copy link
Collaborator Author

geekosaur commented Sep 16, 2024

Yes, as I observed with #10291. But see #10263; @mpickering is insisting it's necessary.

Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go then.

@fgaz
Copy link
Member

fgaz commented Sep 27, 2024

https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstepscontinue-on-error

Prevents a job from failing when a step fails. Set to true to allow a job to pass when this step fails.

Won't all jobs always pass then?

@geekosaur
Copy link
Collaborator Author

I need to verify, but I believe they still get logged as "failure" and the post job catches them.

@mpickering
Copy link
Collaborator

I was suggesting that it would be a improvement to developer experience -- assuming there are a large amount of resources available.

If there are resource bottle-necks I would prefer to run a slimmed down pipeline at first (one ghc version on windows, linux, darwin) before engaging with the full pipeline.

@geekosaur
Copy link
Collaborator Author

We do have larger resources than most free users get. I don't think there's actually a problem here because it's not like it will take more resources than a successful run, unlike changes that require additional rebuilds solely because they can no longer use cached builds.

@geekosaur
Copy link
Collaborator Author

Okay, it looks like jobs succeed when they're not supposed to indeed. This will be a problem.

@geekosaur geekosaur force-pushed the make-it-go branch 5 times, most recently from 54adc8a to 4467b2a Compare September 29, 2024 00:19
@geekosaur
Copy link
Collaborator Author

geekosaur commented Sep 29, 2024

Will changing the workflow to loop within a single step (not job) rather than using separate steps for each test cause problems for anyone? If so, please speak up now.

@ulysses4ever
Copy link
Collaborator

It's not a hard preference but I like the UI of separate steps more, that's why I suggest to use the other solution in that SO thread, based on the if field of a step.

@geekosaur
Copy link
Collaborator Author

Also, I'm not sure this is accomplishing what it's supposed to, because of the way validate.sh runs tests: I think that is aborting on the deliberate lib-suite error and not running the cli-suite tests. That's not going to be fixed by hacking the workflow, and fixing it probably depends on #10320.

@geekosaur
Copy link
Collaborator Author

I (still) don't see a link to said SO, and I'm not sure an if: can solve either problem. You still need to arrange for the whole thing to fail at the end if any step did. Additionally, we already have if:s on the solver tests and would need to integrate those into it without causing them to look like failures if they aren't supposed to run.

@ulysses4ever
Copy link
Collaborator

The SO thread I linked in Matrix a couple weeks ago: https://stackoverflow.com/a/58859404/465100 I think it shouldn't suffer from the same false negatives issue (succeeding workflow having some steps failed) but I wouldn't bet on it: this needs experimenting.

For looping over steps of validate: I don't see a problem if that's a bash-loop and every call to validate.sh with a specific step is independent that way.

@geekosaur
Copy link
Collaborator Author

geekosaur commented Sep 29, 2024

That does the wrong thing: it would, for example, merge with failed tests. if: success() || failure() must be used with great care, and if: always() with even greater care because, as several of the answers there state, you can leave yourself with a job that can't be canceled.

The SO question was to have a step run even if the job was failing. This is incorrect here, because we shouldn't run the tests if the build failed. Moreover, we don't want the job to be counted as a success if a test with that condition failed. There are some very complicated conditionals that could accomplish that in an if:, but the shell script is easier to read especially when the extra condition on the solver tests is included.

@geekosaur geekosaur added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Oct 1, 2024
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Oct 1, 2024
Matt Pickering says we should always run all tests. This requires
a loop to ensure we still fail if any test does.
@mergify mergify bot merged commit 96df6f8 into haskell:master Oct 3, 2024
48 checks passed
@geekosaur
Copy link
Collaborator Author

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Oct 12, 2024

backport 3.12

✅ Backports have been created

@geekosaur
Copy link
Collaborator Author

@mergify backport 3.14

Copy link
Contributor

mergify bot commented Oct 12, 2024

backport 3.14

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous-integration merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants