Skip to content

Commit

Permalink
extract the reporter out of integrity checker (#3248)
Browse files Browse the repository at this point in the history 
* extract the reporter out of integrity checker

* use warning for messages from integrity-checker in check command

* add tests for integrity when integrity file is missing or is not a json

* add assert on pre-existent tests

* add test for integrity failed for linked modules

* delete warning for integrity failed for patterns because we return before if one pattern is missing and we never reach that code

* lint the code correctly

* use flow enum, and remove every Promise.resolve in integrity-checker
  • Loading branch information
@lovelypuppy0607
lovelypuppy0607 committed Apr 26, 2017
1 parent 2943ad0 commit 2163759
Show file tree
Hide file tree
Showing 6 changed files with 135 additions and 76 deletions.
4 changes: 2 additions & 2 deletions __tests__/commands/_helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export async function run<T, R>(
args: Array<string>,
flags: Object,
name: string | { source: string, cwd: string },
checkInstalled: ?(config: Config, reporter: R, install: T) => ?Promise<void>,
checkInstalled: ?(config: Config, reporter: R, install: T, getStdout: () => string) => ?Promise<void>,
beforeInstall: ?(cwd: string) => ?Promise<void>,
): Promise<void> {
let out = '';
Expand Down Expand Up @@ -131,7 +131,7 @@ export async function run<T, R>(
const install = await factory(args, flags, config, reporter, lockfile, () => out);

if (checkInstalled) {
await checkInstalled(config, reporter, install);
await checkInstalled(config, reporter, install, () => out);
}
} catch (err) {
throw new Error(`${err && err.stack} \nConsole output:\n ${out}`);
Expand Down
90 changes: 72 additions & 18 deletions __tests__/commands/check.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const runCheck = buildRun.bind(
},
);

test.concurrent('--verify-tree should report wrong version ', async (): Promise<void> => {
test.concurrent('--verify-tree should report wrong version', async (): Promise<void> => {
let thrown = false;
try {
await runCheck([], {verifyTree: true}, 'verify-tree-version-mismatch');
Expand All @@ -31,7 +31,7 @@ test.concurrent('--verify-tree should report wrong version ', async (): Promise<
expect(thrown).toEqual(true);
});

test.concurrent('--verify-tree should report missing dependency ', async (): Promise<void> => {
test.concurrent('--verify-tree should report missing dependency', async (): Promise<void> => {
let thrown = false;
try {
await runCheck([], {verifyTree: true}, 'verify-tree-not-found');
Expand Down Expand Up @@ -80,6 +80,36 @@ test.concurrent('--integrity should ignore comments and whitespaces in yarn.lock
});
});

test.concurrent('--integrity should fail if integrity file is missing', async (): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'), async (config, reporter): Promise<void> => {
await fs.unlink(path.join(config.cwd, 'node_modules', '.yarn-integrity'));

let thrown = false;
try {
await checkCmd.run(config, reporter, {integrity: true}, []);
} catch (e) {
thrown = true;
}
expect(thrown).toEqual(true);
});
});

test.concurrent('--integrity should fail if integrity file is not a json', async (): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'),
async (config, reporter, install, getStdout): Promise<void> => {
await fs.writeFile(path.join(config.cwd, 'node_modules', '.yarn-integrity'), 'not a json');

let thrown = false;
try {
await checkCmd.run(config, reporter, {integrity: true}, []);
} catch (e) {
thrown = true;
}
expect(thrown).toEqual(true);
expect(getStdout()).toContain('Integrity check: integrity file is not a json');
});
});

test.concurrent('--integrity should fail if yarn.lock has patterns changed', async (): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'), async (config, reporter): Promise<void> => {
let lockfile = await fs.readFile(path.join(config.cwd, 'yarn.lock'));
Expand All @@ -97,7 +127,8 @@ test.concurrent('--integrity should fail if yarn.lock has patterns changed', asy
});

test.concurrent('--integrity should fail if yarn.lock has new pattern', async (): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'), async (config, reporter): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'),
async (config, reporter, install, getStdout): Promise<void> => {
let lockfile = await fs.readFile(path.join(config.cwd, 'yarn.lock'));
lockfile += `\nxtend@^4.0.0:
version "4.0.1"
Expand All @@ -111,11 +142,13 @@ test.concurrent('--integrity should fail if yarn.lock has new pattern', async ()
thrown = true;
}
expect(thrown).toEqual(true);
expect(getStdout()).toContain('Integrity check: Lock files don\'t match');
});
});

test.concurrent('--integrity should fail if yarn.lock has resolved changed', async (): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'), async (config, reporter): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'),
async (config, reporter, install, getStdout): Promise<void> => {
let lockfile = await fs.readFile(path.join(config.cwd, 'yarn.lock'));
lockfile = lockfile.replace('https://registry.npmjs.org/left-pad/-/left-pad-1.1.1.tgz',
'https://registry.yarnpkg.com/left-pad/-/left-pad-1.1.1.tgz');
Expand All @@ -128,13 +161,14 @@ test.concurrent('--integrity should fail if yarn.lock has resolved changed', asy
thrown = true;
}
expect(thrown).toEqual(true);
expect(getStdout()).toContain('Integrity check: Lock files don\'t match');
});
});

test.concurrent('--integrity should fail if files are missing and --check-files is passed',
async (): Promise<void> => {
await runInstall({checkFiles: true}, path.join('..', 'check', 'integrity-lock-check'),
async (config, reporter): Promise<void> => {
async (config, reporter, install, getStdout): Promise<void> => {
await fs.unlink(path.join(config.cwd, 'node_modules', 'left-pad', 'index.js'));

let thrown = false;
Expand All @@ -144,22 +178,23 @@ async (): Promise<void> => {
thrown = true;
}
expect(thrown).toEqual(true);
expect(getStdout()).toContain('Integrity check: Files are missing');
});
});

test.concurrent('--integrity should fail if --ignore-scripts is changed',
async (): Promise<void> => {
await runInstall({ignoreScripts: true}, path.join('..', 'check', 'integrity-lock-check'),
async (config, reporter): Promise<void> => {
let thrown = false;
try {
await checkCmd.run(config, reporter, {integrity: true, ignoreScripts: false}, []);
} catch (e) {
thrown = true;
}
expect(thrown).toEqual(true);
});
});
test.concurrent('--integrity should fail if --ignore-scripts is changed', async (): Promise<void> => {
await runInstall({ignoreScripts: true}, path.join('..', 'check', 'integrity-lock-check'),
async (config, reporter, install, getStdout): Promise<void> => {
let thrown = false;
try {
await checkCmd.run(config, reporter, {integrity: true, ignoreScripts: false}, []);
} catch (e) {
thrown = true;
}
expect(thrown).toEqual(true);
expect(getStdout()).toContain('Integrity check: Flags don\'t match');
});
});

test.concurrent('when switching to --check-files install should rebuild integrity file',
async (): Promise<void> => {
Expand Down Expand Up @@ -204,3 +239,22 @@ async (): Promise<void> => {

});
});

test.concurrent('--integrity should fail if integrity file have different linkedModules', async (): Promise<void> => {
await runInstall({}, path.join('..', 'check', 'integrity-lock-check'),
async (config, reporter, install, getStdout): Promise<void> => {
const integrityFilePath = path.join(config.cwd, 'node_modules', '.yarn-integrity');
const integrityFile = JSON.parse(await fs.readFile(integrityFilePath));
integrityFile.linkedModules.push('aLinkedModule');
await fs.writeFile(integrityFilePath, JSON.stringify(integrityFile, null, 2));

let thrown = false;
try {
await checkCmd.run(config, reporter, {integrity: true}, []);
} catch (e) {
thrown = true;
}
expect(thrown).toEqual(true);
expect(getStdout()).toContain('Integrity check: Linked modules don\'t match');
});
});
6 changes: 4 additions & 2 deletions src/cli/commands/check.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import type Config from '../../config.js';
import {MessageError} from '../../errors.js';
import InstallationIntegrityChecker from '../../integrity-checker.js';
import {integrityErrors} from '../../integrity-checker.js';
import Lockfile from '../../lockfile/wrapper.js';
import type {Reporter} from '../../reporters/index.js';
import * as fs from '../../util/fs.js';
Expand Down Expand Up @@ -147,7 +148,7 @@ async function integrityHashCheck(
reporter.error(reporter.lang(msg, ...vars));
errCount++;
}
const integrityChecker = new InstallationIntegrityChecker(config, reporter);
const integrityChecker = new InstallationIntegrityChecker(config);

const lockfile = await Lockfile.fromDirectory(config.cwd);
const install = new Install(flags, config, reporter, lockfile);
Expand All @@ -162,7 +163,8 @@ async function integrityHashCheck(
if (match.integrityFileMissing) {
reportError('noIntegrityFile');
}
if (!match.integrityMatches) {
if (match.integrityMatches === false) {
reporter.warn(reporter.lang(integrityErrors[match.integrityError]));
reportError('integrityCheckFailed');
}

Expand Down
2 changes: 1 addition & 1 deletion src/cli/commands/install.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ export class Install {

this.resolver = new PackageResolver(config, lockfile);
this.fetcher = new PackageFetcher(config, this.resolver);
this.integrityChecker = new InstallationIntegrityChecker(config, this.reporter);
this.integrityChecker = new InstallationIntegrityChecker(config);
this.compatibility = new PackageCompatibility(config, this.resolver, this.flags.ignoreEngines);
this.linker = new PackageLinker(config, this.resolver);
this.scripts = new PackageInstallScripts(config, this.resolver, this.flags.force);
Expand Down
107 changes: 55 additions & 52 deletions src/integrity-checker.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import type Config from './config.js';
import type {LockManifest} from './lockfile/wrapper.js';
import type {RegistryNames} from './registries/index.js';
import type {Reporter} from './reporters/index.js';
import * as constants from './constants.js';
import {registryNames} from './registries/index.js';
import * as fs from './util/fs.js';
Expand All @@ -13,9 +12,20 @@ import type {InstallArtifacts} from './package-install-scripts.js';
const invariant = require('invariant');
const path = require('path');

export const integrityErrors = {
EXPECTED_IS_NOT_A_JSON: 'integrityFailedExpectedIsNotAJSON',
FILES_MISSING: 'integrityFailedFilesMissing',
LOCKFILE_DONT_MATCH: 'integrityLockfilesDontMatch',
FLAGS_DONT_MATCH: 'integrityFlagsDontMatch',
LINKED_MODULES_DONT_MATCH: 'integrityCheckLinkedModulesDontMatch',
};

type IntegrityError = $Keys<typeof integrityErrors>;

export type IntegrityCheckResult = {
integrityFileMissing: boolean,
integrityMatches?: boolean,
integrityError?: IntegrityError,
missingPatterns: Array<string>,
};

Expand Down Expand Up @@ -47,15 +57,11 @@ type IntegrityFlags = {
export default class InstallationIntegrityChecker {
constructor(
config: Config,
reporter: Reporter,
) {
this.config = config;
this.reporter = reporter;
}

config: Config;
reporter: Reporter;


/**
* Get the location of an existing integrity hash. If none exists then return the location where we should
Expand Down Expand Up @@ -167,32 +173,58 @@ export default class InstallationIntegrityChecker {
return result;
}

_compareIntegrityFiles(actual: IntegrityFile, expected: IntegrityFile): boolean {
if (!compareSortedArrays(actual.linkedModules, expected.linkedModules)) {
this.reporter.warn(this.reporter.lang('integrityCheckLinkedModulesDontMatch'));
return false;
async _getIntegrityFile(locationPath: string): Promise<?IntegrityFile> {
const expectedRaw = await fs.readFile(locationPath);
try {
return JSON.parse(expectedRaw);
} catch (e) {
// ignore JSON parsing for legacy text integrity files compatibility
}
return null;
}

async _compareIntegrityFiles(
actual: IntegrityFile,
expected: ?IntegrityFile,
checkFiles: boolean,
locationFolder: string): Promise<'OK' | IntegrityError> {
if (!expected) {
return 'EXPECTED_IS_NOT_A_JSON';
}
if (!compareSortedArrays(actual.topLevelPatters, expected.topLevelPatters)) {
this.reporter.warn(this.reporter.lang('integrityPatternsDontMatch'));
return false;
if (!compareSortedArrays(actual.linkedModules, expected.linkedModules)) {
return 'LINKED_MODULES_DONT_MATCH';
}
if (!compareSortedArrays(actual.flags, expected.flags)) {
this.reporter.warn(this.reporter.lang('integrityFlagsDontMatch'));
return false;
return 'FLAGS_DONT_MATCH';
}
for (const key of Object.keys(actual.lockfileEntries)) {
if (actual.lockfileEntries[key] !== expected.lockfileEntries[key]) {
this.reporter.warn(this.reporter.lang('integrityLockfilesDontMatch'));
return false;
return 'LOCKFILE_DONT_MATCH';
}
}
for (const key of Object.keys(expected.lockfileEntries)) {
if (actual.lockfileEntries[key] !== expected.lockfileEntries[key]) {
this.reporter.warn(this.reporter.lang('integrityLockfilesDontMatch'));
return false;
return 'LOCKFILE_DONT_MATCH';
}
}
if (checkFiles) {
if (expected.files.length === 0) {
// edge case handling - --check-fies is passed but .yarn-integrity does not contain any files
// check and fail if there are file in node_modules after all.
const actualFiles = await this._getFilesDeep(locationFolder);
if (actualFiles.length > 0) {
return 'FILES_MISSING';
}
} else {
// TODO we may want to optimise this check by checking only for package.json files on very large trees
for (const file of expected.files) {
if (!await fs.exists(path.join(locationFolder, file))) {
return 'FILES_MISSING';
}
}
}
}
return true;
return 'OK';
}

async check(
Expand All @@ -214,41 +246,13 @@ export default class InstallationIntegrityChecker {
patterns,
Object.assign({}, {checkFiles: false}, flags), // don't generate files when checking, we check the files below
loc.locationFolder);
const expectedRaw = await fs.readFile(loc.locationPath);
let expected: ?IntegrityFile;
try {
expected = JSON.parse(expectedRaw);
} catch (e) {
// ignore JSON parsing for legacy text integrity files compatibility
}
let integrityMatches;
if (expected) {
integrityMatches = this._compareIntegrityFiles(actual, expected);
if (flags.checkFiles && expected.files.length === 0) {
// edge case handling - --check-fies is passed but .yarn-integrity does not contain any files
// check and fail if there are file in node_modules after all.
const actualFiles = await this._getFilesDeep(loc.locationFolder);
if (actualFiles.length > 0) {
this.reporter.warn(this.reporter.lang('integrityFailedFilesMissing'));
integrityMatches = false;
}
} else if (flags.checkFiles && expected.files.length > 0) {
// TODO we may want to optimise this check by checking only for package.json files on very large trees
for (const file of expected.files) {
if (!await fs.exists(path.join(loc.locationFolder, file))) {
this.reporter.warn(this.reporter.lang('integrityFailedFilesMissing'));
integrityMatches = false;
break;
}
}
}
} else {
integrityMatches = false;
}
const expected = await this._getIntegrityFile(loc.locationPath);
const integrityMatches = await this._compareIntegrityFiles(actual, expected, flags.checkFiles, loc.locationFolder);

return {
integrityFileMissing: false,
integrityMatches,
integrityMatches: integrityMatches === 'OK',
integrityError: integrityMatches === 'OK' ? undefined : integrityMatches,
missingPatterns,
};
}
Expand Down Expand Up @@ -295,5 +299,4 @@ export default class InstallationIntegrityChecker {
await fs.unlink(loc.locationPath);
}
}

}
2 changes: 1 addition & 1 deletion src/reporters/lang/en.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,8 @@ const messages = {
lockfileNotContainPattern: 'Lockfile does not contain pattern: $0',
integrityCheckFailed: 'Integrity check failed',
noIntegrityFile: 'Couldn\'t find an integrity file',
integrityFailedExpectedIsNotAJSON: 'Integrity check: integrity file is not a json',
integrityCheckLinkedModulesDontMatch: 'Integrity check: Linked modules don\'t match',
integrityPatternsDontMatch: 'Integrity check: Patterns don\'t match',
integrityFlagsDontMatch: 'Integrity check: Flags don\'t match',
integrityLockfilesDontMatch: 'Integrity check: Lock files don\'t match',
integrityFailedFilesMissing: 'Integrity check: Files are missing',
Expand Down

0 comments on commit 2163759

Please sign in to comment.