Skip to content

Add package-lock.json files to user tests #35341

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

Closed
wants to merge 1 commit into from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Nov 25, 2019

Try to use the same package versions that resulted in the baselines. Update the package versions in rolling builds and reuse them in triggered PR associated builds.

@jablko jablko force-pushed the patch-29 branch 3 times, most recently from 768134d to 94ba3e6 Compare November 29, 2019 22:25
@jablko jablko force-pushed the patch-29 branch 3 times, most recently from 9d62246 to 7c6f55a Compare January 25, 2020 20:23
@sandersn sandersn added the Housekeeping Housekeeping PRs label Feb 1, 2020
@jablko jablko force-pushed the patch-29 branch 4 times, most recently from 2e8052f to d69c888 Compare February 6, 2020 14:56
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

This idea seems reasonable, but we need one of us to commit to watch runs after merging.

@weswigham does this look OK to you? Are you able to make sure this works correctly if we decide to merge it?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

After discussing with @weswigham in person, we decided

  1. Lack of package-lock hasn't been a problem for PR builds so far.
  2. I worry that package-locks will cause problems keeping builds up to date.
  3. Neither of us are sure that the mechanism to distinguish PR builds from regularly triggered builds is correct.

So I don't think we'll take this PR.

@@ -67,7 +67,7 @@ namespace Harness {
cwd = config.path ? path.join(cwd, config.path) : submoduleDir;
}
if (fs.existsSync(path.join(cwd, "package.json"))) {
if (fs.existsSync(path.join(cwd, "package-lock.json"))) {
if (process.env.TRAVIS_EVENT_TYPE === "cron" && fs.existsSync(path.join(cwd, "package-lock.json"))) {
Copy link
Member

@weswigham weswigham Feb 7, 2020

Choose a reason for hiding this comment

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

Just leaving the lockfile in place wouldn't be enough to make it get used, it'd also need to be committed (otherwise no PR build is going to have them anyway, as it's always a fresh clone for a CI build), and installation would need to use npm ci rather than npm i (since npm i updates the lockfile by default). Then, for this whole scheme to be usable, we'd need to ensure that the package-lock.json updates are included in and themselves create a "user test baselines updated" PR (meaning we'd need to "fail the build" and open a PR if any package locks update).

Oh, and all this is only actually useful if someone actually reviews those package-lock updates and understands them and the effects they have on the associated builds, rather than always just checkbox-merging them; otherwise we may as well just not have a lockfile, since we're essentially blindly installing the latest allowable packages anyway, and we may as well cut the busywork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes to everything you've said. Would it be possible to include package-lock.json files in the "User test baselines have changed" PRs, but continue to create a PR only if the baselines themselves have changed?

I.e. add them to the existing "User test baselines have changed" PRs, but don't create a PR/fail the build for package-lock.json-only changes?

@jablko jablko force-pushed the patch-29 branch 8 times, most recently from 4b9c727 to d17dd5b Compare February 13, 2020 21:16
@jablko
Copy link
Contributor Author

jablko commented Feb 22, 2020

@typescript-bot user test this

@sandersn
Copy link
Member

@typescript-bot user test this

(sorry, typescript-bot only responds to team members)

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 26, 2020

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at ae92f8f. You can monitor the build here.

@sandersn
Copy link
Member

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

@sandersn sandersn closed this Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Housekeeping Housekeeping PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants