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

[Tests] migrate tests to Github Actions #249

Merged
merged 3 commits into from
Apr 15, 2021
Merged

[Tests] migrate tests to Github Actions #249

merged 3 commits into from
Apr 15, 2021

Conversation

ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 6, 2021

This migrates tests to use github actions, and in so doing adds new node majors, and will automatically add new ones as they're released.

(see ljharb/object.assign#81)

In order for this to land, branch protections must be updated to uncheck travis, and to check the 4 overarching actions status checks that this PR should generate.


steps:
- uses: actions/checkout@v2
- uses: ljharb/actions/node/install@main
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have much experience with GitHub Actions, but having code slurped in like this makes me a bit nervous because it is from an external source and it is not pinned to a specific version. Are these legitimate concerns here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only if you don’t think you can rely on the author maintaining back compat and being a good actor. travis-ci already relies on the same things and auto-updates nvm to whatever version is published.

Happy to peg it to a sha if you prefer, it just means bugfixes or improvements have to be explicitly updated to here rather than automatically flowing in.

@lencioni
Copy link
Collaborator

lencioni commented Apr 8, 2021

@ljharb you mentioned that 4 status checks should be made required as of this PR, but I'm not sure which 4 you are actually referring to. Can you help me out with that?

@ljharb
Copy link
Collaborator Author

ljharb commented Apr 8, 2021

Absolutely:

  1. "Tests: node.js / node 6+"
  2. "Tests: pretest/posttest / pretest"

ah, but the other two have to land in master, and be triggered by a PR after that, before they'll show up:

  1. "Automatic Rebase"
  2. "Require Allow Edits"

so it's totally fine to just add the first two for now.

(travis-ci also needs to be unchecked)

@lencioni lencioni merged commit 918254b into master Apr 15, 2021
@lencioni lencioni deleted the travis branch April 15, 2021 14:28
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