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.

Sign up for GitHub

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 duplicate module detection #7333

Merged
merged 2 commits into from
Nov 7, 2018
+49 −30
Merged

Fix duplicate module detection #7333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2e45e8
Fix duplicate module detection in haste map
rubennorte Nov 6, 2018
7f18a6f
Fix failing test due to differences in contexts
rubennorte Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
- `[jest-haste-map]` Fix to resolve path that is start with words same as rootDir ([#7324](https://github.com/facebook/jest/pull/7324))
- `[expect]` Fix toMatchObject matcher when used with `Object.create(null)` ([#7334](https://github.com/facebook/jest/pull/7334))
- `[jest-haste-map]` [**BREAKING**] Recover files correctly after haste name collisions are fixed ([#7329](https://github.com/facebook/jest/pull/7329))
- `[jest-haste-map]` Remove legacy condition for duplicate module detection ([#7333](https://github.com/facebook/jest/pull/7333))

### Chore & Maintenance

Expand Down
71 changes: 45 additions & 26 deletions packages/jest-haste-map/src/__tests__/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,18 +108,25 @@ const object = data => Object.assign(Object.create(null), data);
const createMap = obj => new Map(Object.keys(obj).map(key => [key, obj[key]]));

// Jest toEqual does not match Map instances from different contexts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjesun do you think we should fix this in Jest matchers? Given it's a very unfrequent use case we might just use this solution.

const normalizePersisted = hasteMap => ({
clocks: normalizeMap(hasteMap.clocks),
duplicates: normalizeMap(hasteMap.duplicates),
files: normalizeMap(hasteMap.files),
map: normalizeMap(hasteMap.map),
mocks: normalizeMap(hasteMap.mocks),
});
const normalizeMap = map => {
if (Object.prototype.toString.call(map) !== '[object Map]') {
throw new TypeError('expected map instance');
// This normalizes them for the uses cases in this test
const useBuitinsInContext = value => {
const stringTag = Object.prototype.toString.call(value);
switch (stringTag) {
case '[object Map]':
return new Map(
Array.from(value).map(([k, v]) => [
useBuitinsInContext(k),
useBuitinsInContext(v),
]),
);
case '[object Object]':
return Object.keys(value).reduce((obj, key) => {
obj[key] = useBuitinsInContext(value[key]);
return obj;
}, {});
default:
return value;
}
return new Map(map);
};

let consoleWarn;
Expand Down Expand Up @@ -359,7 +366,7 @@ describe('HasteMap', () => {

// The cache file must exactly mirror the data structure returned from a
// build
expect(normalizePersisted(hasteMap.read())).toEqual(data);
expect(useBuitinsInContext(hasteMap.read())).toEqual(data);
});
});

Expand Down Expand Up @@ -433,7 +440,7 @@ describe('HasteMap', () => {
}),
);

expect(normalizePersisted(hasteMap.read())).toEqual(data);
expect(useBuitinsInContext(hasteMap.read())).toEqual(data);
});
});
});
Expand Down Expand Up @@ -523,6 +530,18 @@ describe('HasteMap', () => {
});
});

it('warns on duplicate module ids only once', async () => {
mockFs['/project/fruits/other/Strawberry.js'] = `
const Banana = require("Banana");
`;

await new HasteMap(defaultConfig).build();
expect(console.warn).toHaveBeenCalledTimes(1);

await new HasteMap(defaultConfig).build();
expect(console.warn).toHaveBeenCalledTimes(1);
});

it('throws on duplicate module ids if "throwOnModuleCollision" is set to true', () => {
// Raspberry thinks it is a Strawberry
mockFs['/project/fruits/another/Strawberry.js'] = `
Expand Down Expand Up @@ -615,9 +634,9 @@ describe('HasteMap', () => {
} else {
expect(fs.readFileSync).toBeCalledWith(cacheFilePath, 'utf8');
}
expect(normalizeMap(data.clocks)).toEqual(mockClocks);
expect(normalizeMap(data.files)).toEqual(initialData.files);
expect(normalizeMap(data.map)).toEqual(initialData.map);
expect(useBuitinsInContext(data.clocks)).toEqual(mockClocks);
expect(useBuitinsInContext(data.files)).toEqual(initialData.files);
expect(useBuitinsInContext(data.map)).toEqual(initialData.map);
});
}));

Expand Down Expand Up @@ -655,15 +674,15 @@ describe('HasteMap', () => {
'utf8',
);

expect(normalizeMap(data.clocks)).toEqual(mockClocks);
expect(useBuitinsInContext(data.clocks)).toEqual(mockClocks);

const files = new Map(initialData.files);
files.set('fruits/Banana.js', ['Banana', 32, 1, ['Kiwi'], null]);

expect(normalizeMap(data.files)).toEqual(files);
expect(useBuitinsInContext(data.files)).toEqual(files);

const map = new Map(initialData.map);
expect(normalizeMap(data.map)).toEqual(map);
expect(useBuitinsInContext(data.map)).toEqual(map);
});
}));

Expand All @@ -690,11 +709,11 @@ describe('HasteMap', () => {
.then(({__hasteMapForTest: data}) => {
const files = new Map(initialData.files);
files.delete('fruits/Banana.js');
expect(normalizeMap(data.files)).toEqual(files);
expect(useBuitinsInContext(data.files)).toEqual(files);

const map = new Map(initialData.map);
map.delete('Banana');
expect(normalizeMap(data.map)).toEqual(map);
expect(useBuitinsInContext(data.map)).toEqual(map);
});
}));

Expand Down Expand Up @@ -781,7 +800,7 @@ describe('HasteMap', () => {
const {__hasteMapForTest: data} = await new HasteMap(
defaultConfig,
).build();
expect(normalizeMap(data.duplicates)).toEqual(
expect(useBuitinsInContext(data.duplicates)).toEqual(
createMap({
Strawberry: createMap({
g: createMap({
Expand All @@ -807,7 +826,7 @@ describe('HasteMap', () => {
const {__hasteMapForTest: data} = await new HasteMap(
defaultConfig,
).build();
expect(normalizeMap(data.duplicates)).toEqual(new Map());
expect(useBuitinsInContext(data.duplicates)).toEqual(new Map());
expect(data.map.get('Strawberry')).toEqual({
g: ['fruits/Strawberry.js', H.MODULE],
});
Expand All @@ -826,7 +845,7 @@ describe('HasteMap', () => {
defaultConfig,
).build();

expect(normalizeMap(data.duplicates)).toEqual(
expect(useBuitinsInContext(data.duplicates)).toEqual(
createMap({
Strawberry: createMap({
g: createMap({
Expand All @@ -853,7 +872,7 @@ describe('HasteMap', () => {
defaultConfig,
).build();

expect(normalizeMap(correctData.duplicates)).toEqual(new Map());
expect(useBuitinsInContext(correctData.duplicates)).toEqual(new Map());
expect(correctData.map.get('Strawberry')).toEqual({
g: ['fruits/Strawberry.js', H.MODULE],
});
Expand All @@ -874,7 +893,7 @@ describe('HasteMap', () => {
const {__hasteMapForTest: data} = await new HasteMap(
defaultConfig,
).build();
expect(normalizeMap(data.duplicates)).toEqual(new Map());
expect(useBuitinsInContext(data.duplicates)).toEqual(new Map());
expect(data.map.get('Strawberry')).toEqual({
g: ['fruits/Strawberry.js', H.MODULE],
});
Expand Down
7 changes: 3 additions & 4 deletions packages/jest-haste-map/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,9 @@ class HasteMap extends EventEmitter {
cachedFiles.push({moduleName, path: relativeFilePath});
}
return this._crawl(cachedHasteMap).then(hasteMap => {
const deprecatedFiles = cachedFiles.filter(file => {
const fileData = hasteMap.files.get(file.path);
return fileData == null || file.moduleName !== fileData[H.ID];
});
const deprecatedFiles = cachedFiles.filter(
file => !hasteMap.files.has(file.path),
);
return {deprecatedFiles, hasteMap};
});
});
Expand Down