Fix #6029 Avoid undefined checksums after merge conflict resolution

[ReDrUm]

What's the problem this PR addresses?

Resolves #6029

Running yarn --mode=update-lockfile during git conflict resolution, performs autofixMergeConflicts. If the yarn.lock file has entries that lack a checksum value, that process introduces an undefined checksum value which is likely to be committed by accident. Subsequent full installs will remove them, but they're a cause of friction during their temporary existence.

In my case under Yarn 4.5.3 using nodeLinker: node-modules it was injecting checksum: 10/undefined, while in #6029 it was injecting checksum: undefined. I note the __metadata.cacheKey prefix (10/ in my case) can vary from project to project.

...

How did you fix it?

Explicitly checking for undefined in the autofixMergeConflicts method.

I struggled to figure out a way to add test coverage for this. I tried to add an extra dependency into the it should properly fix merge conflicts test case within packages/acceptance-tests/pkg-tests-specs/sources/features/mergeConflictResolution.test.ts, however it always injects a checksum value, so i'm not sure how to emulate a missing one? Open to guidance there.

In my project's yarn.lock most entries do have a checksum, while some do not, as a mixture of npm: and workspace: protocol entries, so i'm not sure what the rules are for when the checksum is added?

Manual QA

I tested this by updating my project's .yarnrc.yaml yarnPath to point to scripts/run-yarn.js from my local yarn-berry checkout. Then in my external project i replicated a git conflict within a yarn.lock entry, and used yarn --mode=update-lockfile to validate my fix correctly avoided introducing checksum: 10/undefined for entries lacking a checksum value.

...

Checklist

• [ x ] I have read the Contributing Guide.

• [ x ] I have set the packages that need to be released for my changes to be effective.

If i've selected the wrong packages please let me know. Given this PR touches a commonly used package i opted for a partial release rather than all dependents. Happy to change based on reviewer feedback.

• [ x ] I will check that all automated PR checks pass before the PR gets reviewed.

[arcanis]

I struggled to figure out a way to add test coverage for this. I tried to add an extra dependency into the it should properly fix merge conflicts test case within packages/acceptance-tests/pkg-tests-specs/sources/features/mergeConflictResolution.test.ts, however it always injects a checksum value, so i'm not sure how to emulate a missing one? Open to guidance there.

Did you try hardcoding a lockfile without checksums in the test? iirc the main case for checksums being missing is when the lockfile references an optional dependency with incompatible libc / os fields (we can't compute the checksum, since we never download the file).

[ReDrUm]

Thanks for the context. I'll try hard coding an optional dependency as a new test case.

[ReDrUm]

I played around with a hard coded lockfile and also using some of the dep fixtures like optional-native and optional-peer-deps and sure enough, those entries don't include checksums, but i can't figure out how to replicate the injection of the checksum: <cacheKey>/undefined. This despite using await expect(run('--mode=update-lockfile')).resolves.toMatchSnapshot(); instead of a normal install (because it's update lockfile mode where the problem arises, while a normal install self heals from the phenomenon).

I would have expected that by removing the "fix", and commenting out the stripped checksum regex within cleanLockfile that it should inject checksum: <cacheKey>/undefined into the optional entries (as it does in various projects outside of this repo), but it doesn't. If i console log inside autofixMergeConflicts i can see it is injecting checksum: <cacheKey>/undefined for them, but something else later in the process removes them again, thwarting the usefulness of the test case.

Any ideas? Or, are we happy to merge this fix in isolation without the added test coverage due to the simplicity of the change? It's uniform with other places where checksums are set which also do an existence check.

[arcanis]

It feels innocent enough, but I'm a little worried this could regress in the future. Worst case it'll be another PR 😅