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

Fix yarn check --integrity behavior not aligning with yarn install's integrity checking #2024

Merged
merged 1 commit into from Nov 29, 2016

Conversation

bouk
Copy link
Contributor

@bouk bouk commented Nov 24, 2016

Summary

There's a mismatch between what yarn install does for integrity checking and what yarn check does. yarn check flattens the patterns, while yarn check doesn't, as it just uses Install.hydrate (which doesn't return the patterns flattened)

We ran into some issues where yarn install would say everything was file while yarn check --integrity would say there's a mismatch. This patch fixes it

@bouk bouk changed the title Fix yarn check --integrity Fix yarn check --integrity behavior not aligning with yarn install's integrity checking Nov 24, 2016
@bestander
Copy link
Member

@bouk, that is a great find, thanks!
Do you mind adding a test case so that it does not break in the future?

@bouk
Copy link
Contributor Author

bouk commented Nov 25, 2016

There don't seem to be any tests for check currently, but I'll see what I can do

@bestander
Copy link
Member

You can use the install.js tests for the check function

@bouk
Copy link
Contributor Author

bouk commented Nov 25, 2016

I have added a test that checks whether a yarn install --flat followed by a yarn check --integrity --flat succeeds

Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

Awesome work!
Could you just add an explicit integrity check to the test?

test.concurrent('check and install should verify integrity in the same way when flat', (): Promise<void> => {
// A@2.0.1 -> B@2.0.0
// should result in B@2.0.0 flattened
return runInstall({flat: true, integrity: true}, 'install-should-dedupe-avoiding-conflicts-1', async (config) => {
Copy link
Member

Choose a reason for hiding this comment

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

runInstall has an implicit check to verify that all install operations are consistent.
It would be better to run an explicit check with integrity flag because we may refactor runInstall eventually and this test would be hard to keep consistent.

@bouk
Copy link
Contributor Author

bouk commented Nov 28, 2016

@bestander done!

@@ -23,7 +23,7 @@ export const runInstall = run.bind(
async (args, flags, config, reporter, lockfile): Promise<Install> => {
const install = new Install(flags, config, reporter, lockfile);
await install.init();
await check(config, reporter, {}, []);
await check(config, reporter, flags, []);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should pass flags for install and check via the same variable.
Can you please revert this change?

@bestander bestander merged commit caa94a6 into yarnpkg:master Nov 29, 2016
@bestander
Copy link
Member

@bouk. thanks, great job!

@bouk bouk deleted the fix-yarn-check branch August 9, 2018 13:34
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.

None yet

2 participants