-
Notifications
You must be signed in to change notification settings - Fork 629
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
Add gh actions for building and testing the repo #1310
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1310 +/- ##
=======================================
Coverage 83.93% 83.93%
=======================================
Files 208 208
Lines 10954 10954
Branches 2757 2757
=======================================
Hits 9194 9194
Misses 1760 1760 ☔ View full report in Codecov by Sentry. |
954ee65
to
621a2b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on! Just a couple of nits/questions
41a8980
to
270f4cf
Compare
0e91dc7
to
4085dde
Compare
default: '18.12' | ||
current-node-version: | ||
required: false | ||
default: '20.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon we can reduce to 18.x
, 20.x
as specifiers at the GitHub level so we can simplify this. Then we could receive one node-version
arg as a string, and more easily test 3 versions in future if we need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have a test on 18.0
though since we claim to support it, and there are some differences vs later minors. ['current', 'lts/*', 'lts/-1', '18.0']
(which resolves to 21.x, 20.x, 18.x, 18.0), IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, however @huntie disagreed that it makes sense to test the minimum and lts versions before. His argument regarding lts/*
and I assume current
is that these might "force us to make a CI config code change to unblock future lands" if there's a failure. Instead we should hardcode the versions and bump them manually.
Did I understand you correctly @huntie?
BTW @robhogan, current
resolves to v22.6.0
.
https://github.com/facebook/metro/actions/runs/10286511772/job/28467353526
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fair - essentially it's unstable (though fwiw these CI signals aren't land blocking yet). I'd like to test 18.0 (which should be stable), I'd be OK to use 20.x
, 18.x
instead of the aliases.
It would still be good to know asap if we're incompatible with a new Node major, because we don't put a "maximum" constraint on engines
so Node releases can break users (it's happened before), but ideally we'd run that as a separate scheduled (eg nightly) run rather than anything that potentially signals a diff-time failure unrelated to the diff. We can follow up with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Great idea with the nightlies!
I've updated the code to ['18.0', '18.x', '20.x']
and added to my TODO list to introduce a nightly job that tests lts/*
and current
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more for the TODO list then - we should also have a scheduled test that doesn't use (maybe deletes) yarn.lock
and pulls the latest versions of all our dependencies, so we can see if a dependency update breaks us somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huntie please approve as well
8e864dd
to
55992a4
Compare
55992a4
to
c35b1a6
Compare
@vzaidman LGTM, thanks for iterating - if you want to import I think the diff should now be auto-stamped because I've approved it here (never actually tried that tbh!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! ✅
name: "Tests with coverage" | ||
steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, this indentation looks wrong. Can we triple-check this, and enable Prettier on YAML files in our repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have validation on yml.
Both idents are valid in yml apparently: https://stackoverflow.com/questions/17014460/yaml-indentation-for-array-in-hash
Also, evidently it works.
However, I'll make sure they are formatted the same across the file regardless. Thx for noticing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but can we please enable auto-formatting? 🥹 Good to go 👍🏻
c35b1a6
to
3c91124
Compare
@vzaidman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Introduced config that triggers GH actions that build, lint, and test the repo.
These are triggered when a pull request is created or it's latest commit is updated.
If a new job is triggered, the previous is abandoned.
Test: