Add package-lock.json files to user tests#35341
Uh oh!
There was an error while loading. Please reload this page.
Conversation
jablkoforce-pushed the patch-29 branch 3 times, most recently from 768134d to 94ba3e6Comparejablkoforce-pushed the patch-29 branch 3 times, most recently from 9d62246 to 7c6f55aCompare
jablkoforce-pushed the patch-29 branch 4 times, most recently from 2e8052f to d69c888Compare
sandersn left a comment
sandersn left a comment There was a problem hiding this comment.
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?
sandersn left a comment
sandersn left a comment There was a problem hiding this comment.
After discussing with @weswigham in person, we decided
- Lack of package-lock hasn't been a problem for PR builds so far.
- I worry that package-locks will cause problems keeping builds up to date.
- 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.
| } | ||
| 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"))){ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
jablkoforce-pushed the patch-29 branch 8 times, most recently from 4b9c727 to d17dd5bComparejablko commented Feb 22, 2020
@typescript-bot user test this |
sandersn commented Feb 26, 2020
@typescript-bot user test this (sorry, typescript-bot only responds to team members) |
sandersn commented Mar 11, 2020
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. |
typescript-bot commented Oct 21, 2025
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
4 participants
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.