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

[ui, ci] Ember Exam failures should cause overall test failures #24603

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

philrenaud
Copy link
Contributor

Description

Noticed that failing ember-exam tests were still yielding green checkmarks (see ember exam in https://github.com/hashicorp/nomad/actions/runs/12143968210/job/33862207314)

This moves the continue-on-error declaration out of the sub block and:

  1. allows all tests in a partition to run to completion, even if one errors (up to 30 minutes)
  2. has an explicit "Did something fail?" check step to throw the wider failure.

This builds on the work done in #24555

@philrenaud philrenaud self-assigned this Dec 4, 2024
@philrenaud philrenaud requested review from a team as code owners December 4, 2024 14:40
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

I have questions! (because github actions can be quite confusing)

@@ -24,7 +24,6 @@
{{content-for "body"}}

<script src="{{rootURL}}assets/vendor.js"></script>

Copy link
Member

Choose a reason for hiding this comment

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

🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because of

    paths:
      - "ui/**"

But I'm going to try to make this workflow file part of that, too.

if: steps.exam.outcome != 'success'
run: |
echo "Tests failed or timed out in partition ${{ matrix.partition }}"
exit 1
Copy link
Member

Choose a reason for hiding this comment

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

if we're checking the exam step's outcome specifically, do we need "Express timeout failure" above?

also, if you exit 1 here, the Upload step below wouldn't run. presumably you'd want the test results even if something failed?

but also, does continue-on-error at the job level, moved up to line 39, mean that all steps are continued, so this exit 1 would be ignored just like any error that yarn exam might have spit out?

I don't recall exactly the nuances of when which type of results are skipped, and I don't think I've done a job-level continue-on-error... if you're not sure either, I'd suggest making a little repo that's just for testing github actions, and creating little toy workflows so you can iterate more quickly. I suppose you could do that in a branch in this repo, but I like to do it in branches in my own lil experiment repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, if you exit 1 here, the Upload step below wouldn't run. presumably you'd want the test results even if something failed?

I think probably I wouldn't want to upload the results in that case, actually; the point of uploading partition results outside of the running test step is to do look-backs and measure test timing, etc.

Although: if I also wanted to measure flakiness, I guess I would still want to upload things if there was a failure.

I think I need to think about this a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants