Skip to content

Conversation

mbg
Copy link
Member

@mbg mbg commented Sep 25, 2025

Improves the pr-checks workflow so that:

  • All checks run, even if one fails.
  • A diff of the changes to the JS bundles is added as job summary if there are changes
  • A diff of the changes to the generated workflows is added as job summary if there are changes

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

@mbg mbg requested a review from henrymercer September 25, 2025 12:30
@mbg mbg requested a review from a team as a code owner September 25, 2025 12:30
@mbg mbg requested review from Copilot and removed request for a team September 25, 2025 12:30
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the pr-checks workflow by ensuring all checks run even if one fails and adds helpful diff summaries to job outputs when there are changes to JS bundles or generated workflows.

  • Adds if: always() conditions to allow all checks to run regardless of previous failures
  • Implements diff output to GitHub step summaries for both JS bundle changes and generated workflow changes
  • Includes a git checkout command to reset bundled files after displaying the diff

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
.github/workflows/pr-checks.yml Adds if: always() conditions to ensure all workflow steps run
.github/workflows/script/check-js.sh Adds JS diff output to step summary and resets bundled files
.github/workflows/script/verify-pr-checks.sh Adds generated workflows diff output to step summary

Comment on lines +23 to +24
git diff --output="$RUNNER_TEMP/js.diff"
cat "$RUNNER_TEMP/js.diff" >> $GITHUB_STEP_SUMMARY
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider uploading the diff as an artifact

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially had that, but thought this was nicer because you can see the syntax-highlighted diff without having to download a file, unpack it, and open it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think both are nice in case there's a big diff. But this is a very minor comment, good to go as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's leave it for now then, but we can add it later if we find it's needed?

Comment on lines +27 to +28
git diff --output="$RUNNER_TEMP/workflows.diff"
cat "$RUNNER_TEMP/workflows.diff" >> $GITHUB_STEP_SUMMARY
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here

@henrymercer
Copy link
Contributor

A common gotcha is having out of date dependencies locally. We could consider posting a comment on the PR with the command to run (e.g. npm i && npm run build && npm test && npm run lint). We could also consider integrating a check that dependenices are up to date as part of the build process, if this is possible to do quickly.

@henrymercer
Copy link
Contributor

I'm not super keen on running npm i as part of npm run build just because of how slow it is, but this is also something we could consider.

@mbg
Copy link
Member Author

mbg commented Sep 25, 2025

Agreed on both points. I don't think we should run npm install as part of anything we do frequently during normal development.

I'll have a look at what we can do to detect whether npm install has to be run, but separately from this PR.

@mbg mbg merged commit 8fca381 into main Sep 25, 2025
279 of 322 checks passed
@mbg mbg deleted the mbg/ci/improve-unit-tests branch September 25, 2025 13:21
@github-actions github-actions bot mentioned this pull request Sep 26, 2025
8 tasks
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