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

Only run FE unit on 18 & run build in matrix #15980

Merged
merged 2 commits into from Mar 7, 2023

Conversation

joshuaellis
Copy link
Member

@joshuaellis joshuaellis commented Mar 3, 2023

What does it do?

  • remove running the unit_front tests in node matrix
  • adds build check in all node versions we support

Why is it needed?

  • the unit tests run in jsdom they don't use node APIs therefore the node version has no impact, we can reduce time.
  • the build however, does need to work across all node versions

Related issue(s)/PR(s)

  • mentioned in the FE sync
  • resolves CONTENT-1129

Note

We'll need the required checks to be changed on the branches before merging this, cc @alexandrebodin

@joshuaellis joshuaellis added source: tooling Source is GitHub tooling/tests/ect pr: chore This PR contains chore tasks (cleanups, configs, tooling...) labels Mar 3, 2023
@joshuaellis joshuaellis added this to the 4.7.2 milestone Mar 3, 2023
@joshuaellis joshuaellis self-assigned this Mar 3, 2023
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: +9.07 🎉

Comparison is base (6b493cd) 51.61% compared to head (5003a8c) 60.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15980      +/-   ##
==========================================
+ Coverage   51.61%   60.68%   +9.07%     
==========================================
  Files         374     1495    +1121     
  Lines       14091    36878   +22787     
  Branches     3168     7359    +4191     
==========================================
+ Hits         7273    22380   +15107     
- Misses       5617    12417    +6800     
- Partials     1201     2081     +880     
Flag Coverage Δ
back 51.34% <ø> (-0.02%) ⬇️
front 66.30% <ø> (?)
unit_back 51.34% <ø> (-0.02%) ⬇️
unit_front 66.30% <ø> (?)

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

Impacted Files Coverage Δ
...ckages/core/strapi/lib/commands/transfer/import.js 67.92% <0.00%> (-1.17%) ⬇️
...ckages/core/strapi/lib/commands/transfer/export.js 69.35% <0.00%> (-0.96%) ⬇️
...ages/core/strapi/lib/commands/transfer/transfer.js 64.58% <0.00%> (-0.73%) ⬇️
...ackages/core/admin/server/controllers/api-token.js 93.75% <0.00%> (-0.10%) ⬇️
...dmin/admin/src/components/Notifications/reducer.js 100.00% <0.00%> (ø)
.../i18n/admin/src/components/ModalCreate/BaseForm.js 0.00% <0.00%> (ø)
...e/admin/admin/src/hooks/useConfigurations/index.js 100.00% <0.00%> (ø)
...e-builder/admin/src/components/PluginIcon/index.js 0.00% <0.00%> (ø)
...ingsPage/pages/Webhooks/ProtectedListView/index.js 0.00% <0.00%> (ø)
...es/plugins/users-permissions/admin/src/pluginId.js 100.00% <0.00%> (ø)
... and 1116 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -97,6 +97,26 @@ jobs:
directory: ./coverage
flags: front,unit_front

build:
name: 'build (node: ${{ matrix.node }})'
needs: [lint, unit_front]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this only depend on the lint job?

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 thought about this, but my pov was "no point building if your tests dont pass", wdyt?

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM

@joshuaellis
Copy link
Member Author

LGTM

Thanks @alexandrebodin, would you be able to change the required permissions for branch protection so we no longer want to wait for 14 & 16 but we do for unit_front (node: 18) 🙏🏼

@alexandrebodin
Copy link
Member

@joshuaellis good to go :)

@joshuaellis joshuaellis merged commit 4541486 into main Mar 7, 2023
@joshuaellis joshuaellis deleted the chore/unit-front-node-matrix branch March 7, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: tooling Source is GitHub tooling/tests/ect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants