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

ci(js): gate expensive e2e matrices and builds on unit test pass #9908

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Apr 7, 2022

Overview

After the huge CI runner backlog today caused by a performance degradation in Apple's notarization service, I have a few proposals for reducing our CI load. This PR is one of them!

This PR gates more expensive tests in the front-end CI workflows behind passing the initial unit test job. This has a couple downsides:

  1. Unit tests must pass for sandbox assets to be built and published. If you're working out of sandbox builds, you must ensure the unit test suite is passing before pushing
  2. These workflows will be a little less parallel, and may take several minutes longer (i.e. the length of time the unit tests take to run)

For (1), I'd posit that we're ignoring the vast majority of these builds, anyway, especially if the unit tests aren't passing. If you are relying on the ability of sandbox builds to build with a failing test suite, you... shouldn't be doing that.

For (2), I don't think our workflow time is the thing that's getting us, it's machine usage. This strategy will tie up fewer machines.

Overall, I think the upside for all developers (not just FE devs) in fewer CI machines getting tied up greatly outweighs the downsides of this approach.

Changelog

  • Gate expensive tests and builds behind the passing of unit tests
    • App-shell OS-matrix tests + app OS-matrix builds
    • Component library storybook builds
    • Labware library OS-matrix E2E tests
    • Labware library builds
  • Add 30 minute timeout to app build step
    • Will fail the job if notarization takes too long
    • Today I found a job waiting more then 5 hours for notarization
    • Default job timeout in GH Actions is 6 hours
  • Remove duplicate Labware Library E2E workflow
    • The same exact E2E matrix was already present in the regular LL workflow
  • Remove some redundant branch: "*" filters that weren't doing anything Update branch filters to ** to accept branches including slashes

Review requests

👍 if you think these changes make sense

Risk assessment

Low. Increasing the strictness of our CI jobs

@mcous mcous added the chore label Apr 7, 2022
@mcous mcous requested a review from a team as a code owner April 7, 2022 00:01
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #9908 (7ac5606) into edge (3bffb41) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #9908      +/-   ##
==========================================
+ Coverage   74.99%   75.02%   +0.02%     
==========================================
  Files        2021     2023       +2     
  Lines       53566    53618      +52     
  Branches     5221     5231      +10     
==========================================
+ Hits        40174    40228      +54     
+ Misses      12353    12344       -9     
- Partials     1039     1046       +7     
Flag Coverage Δ
app 71.59% <ø> (ø)
components 52.70% <ø> (+0.21%) ⬆️
labware-library 49.74% <ø> (+0.03%) ⬆️
protocol-designer 44.68% <ø> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...tocol-designer/src/file-data/selectors/commands.ts 31.57% <0.00%> (-0.57%) ⬇️
components/src/modals/BaseModal.tsx 91.66% <0.00%> (ø)
labware-library/src/definitions.tsx 0.00% <0.00%> (ø)
components/src/ui-style-constants/colors.ts 100.00% <0.00%> (ø)
...ocol-designer/src/components/modules/ModuleRow.tsx 92.30% <0.00%> (ø)
...col-designer/src/components/StepEditForm/index.tsx 0.00% <0.00%> (ø)
components/src/icons/ModuleIcon.tsx 25.00% <0.00%> (ø)
...als/AutoAddPauseUntilHeaterShakerTempStepModal.tsx 100.00% <0.00%> (ø)
...rotocol-designer/src/step-forms/selectors/index.ts 43.90% <0.00%> (+2.28%) ⬆️
protocol-designer/src/steplist/actions/actions.ts 71.87% <0.00%> (+3.12%) ⬆️
... and 5 more

@sfoster1
Copy link
Member

sfoster1 commented Apr 7, 2022

@Opentrons/app-and-uis @mcous Do you think it makes sense to make sandbox builds and full app builds actually opt-in with comment handler workflows? How many FE prs do we actually use the CI artifacts from vs local builds?

@mcous
Copy link
Contributor Author

mcous commented Apr 7, 2022

It's an interesting question. I think the main benefit of the sandbox builds isn't the sandbox publish, but the fact that CI is veryfying the change set not only passes tests, but doesn't break the build. The publish to sandbox is almost incidental, but also the lightest-weight part of the check

@SyntaxColoring
Copy link
Contributor

SyntaxColoring commented Apr 7, 2022

Remove some redundant branch: "*" filters that weren't doing anything

I don't know about these workflows specifically, but those filters might have actually been doing something despite looking redundant.

See this note in another workflow:

branches:
# We don't want to do any filtering based on branch name.
# But because we specified `tags`, we need to specify `branches` too,
# or else GitHub will only run this workflow for tag matches
# and not for path matches.
- '**'

Added in this PR.

Also note the ** wildcard instead of *. I haven't tested this, but it might be necessary to handle branch names with a slash in them, like feat/foo. See the wildcard docs.

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

These changes all sound reasonable to me ⚖️

@mcous mcous merged commit a5ecacb into edge Apr 13, 2022
@mcous mcous deleted the ci_fewer-runs branch April 13, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants