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

Fixing Race Condition incase of Screenshot Cmd #1793

Merged
merged 2 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 7 additions & 4 deletions packages/core/src/snapshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import {
hostnameMatches,
yieldTo,
snapshotLogName,
decodeAndEncodeURLWithLogging
decodeAndEncodeURLWithLogging,
compareObjectTypes
} from './utils.js';
import { JobData } from './wait-for-job.js';

Expand Down Expand Up @@ -364,9 +365,11 @@ export function createSnapshotsQueue(percy) {
}
})
// snapshots are unique by name and testCase both
.handle('find', ({ name, testCase }, snapshot) => (
snapshot.testCase === testCase && snapshot.name === name
))
.handle('find', ({ name, testCase, tag }, snapshot) => {
return snapshot.testCase === testCase &&
snapshot.name === name &&
compareObjectTypes(tag, snapshot.tag);
})
// when pushed, maybe flush old snapshots or possibly merge with existing snapshots
.handle('push', (snapshot, existing) => {
let { name, meta } = snapshot;
Expand Down
16 changes: 16 additions & 0 deletions packages/core/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -512,3 +512,19 @@ export class DefaultMap extends Map {
return super.get(key);
};
};

export function compareObjectTypes(obj1, obj2) {
if (obj1 === obj2) return true; // Handles primitives
this-is-shivamsingh marked this conversation as resolved.
Show resolved Hide resolved
if (typeof obj1 !== 'object' || typeof obj2 !== 'object' || obj1 === null || obj2 === null) return false;

const keys1 = Object.keys(obj1);
const keys2 = Object.keys(obj2);

if (keys1.length !== keys2.length) return false;

for (const key of keys1) {
if (!keys2.includes(key) || !compareObjectTypes(obj1[key], obj2[key])) return false;
}
this-is-shivamsingh marked this conversation as resolved.
Show resolved Hide resolved

return true;
}
107 changes: 106 additions & 1 deletion packages/core/test/utils.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { decodeAndEncodeURLWithLogging, waitForSelectorInsideBrowser } from '../src/utils.js';
import { decodeAndEncodeURLWithLogging, waitForSelectorInsideBrowser, compareObjectTypes } from '../src/utils.js';
import { logger, setupTest } from './helpers/index.js';
import percyLogger from '@percy/logger';
import Percy from '@percy/core';
Expand Down Expand Up @@ -73,4 +73,109 @@ describe('utils', () => {
expect(error).toEqual(expectedError);
});
});
describe('compareObjectTypes', () => {
describe('Primitive comparisons', () => {
it('should return true for identical numbers', () => {
expect(compareObjectTypes(42, 42)).toBe(true);
});

it('should return false for different numbers', () => {
expect(compareObjectTypes(42, 43)).toBe(false);
});

it('should return true for identical strings', () => {
expect(compareObjectTypes('hello', 'hello')).toBe(true);
});

it('should return false for different strings', () => {
expect(compareObjectTypes('hello', 'world')).toBe(false);
});

it('should return true for null compared with null', () => {
expect(compareObjectTypes(null, null)).toBe(true);
});

it('should return true for undefined compared with undefined', () => {
expect(compareObjectTypes(undefined, undefined)).toBe(true);
});

it('should return false for null compared with an object', () => {
expect(compareObjectTypes(null, { a: 1 })).toBe(false);
});
});

describe('Shallow object comparisons', () => {
it('should return true for identical shallow objects', () => {
const obj1 = { a: 1, b: 2 };
const obj2 = { a: 1, b: 2 };
expect(compareObjectTypes(obj1, obj2)).toBe(true);
});

it('should return false for objects with different keys', () => {
const obj1 = { a: 1, b: 2 };
const obj2 = { a: 1, c: 2 };
expect(compareObjectTypes(obj1, obj2)).toBe(false);
});

it('should return false for objects with different values', () => {
const obj1 = { a: 1, b: 2 };
const obj2 = { a: 1, b: 3 };
expect(compareObjectTypes(obj1, obj2)).toBe(false);
});
});

describe('Deep object comparisons', () => {
it('should return true for deeply nested identical objects', () => {
const obj1 = { a: { b: { c: 1 } } };
const obj2 = { a: { b: { c: 1 } } };
expect(compareObjectTypes(obj1, obj2)).toBe(true);
});

it('should return false for deeply nested objects with different values', () => {
const obj1 = { a: { b: { c: 1 } } };
const obj2 = { a: { b: { c: 2 } } };
expect(compareObjectTypes(obj1, obj2)).toBe(false);
});
});

describe('Array comparisons', () => {
it('should return true for identical arrays', () => {
const obj1 = [1, 2, 3];
const obj2 = [1, 2, 3];
expect(compareObjectTypes(obj1, obj2)).toBe(true);
});

it('should return false for arrays with different elements', () => {
const obj1 = [1, 2, 3];
const obj2 = [1, 2, 4];
expect(compareObjectTypes(obj1, obj2)).toBe(false);
});

it('should handle mixed structures of arrays and objects', () => {
const obj1 = { a: [1, { b: 2 }], c: 3 };
const obj2 = { a: [1, { b: 2 }], c: 3 };
expect(compareObjectTypes(obj1, obj2)).toBe(true);
});
});

describe('Edge cases', () => {
it('should return false for mismatched data types', () => {
const obj1 = { a: 1 };
const obj2 = 'string';
expect(compareObjectTypes(obj1, obj2)).toBe(false);
});

it('should return true for empty objects', () => {
const obj1 = {};
const obj2 = {};
expect(compareObjectTypes(obj1, obj2)).toBe(true);
});

it('should return false for objects with different key lengths', () => {
const obj1 = { a: 1 };
const obj2 = { a: 1, b: 2 };
expect(compareObjectTypes(obj1, obj2)).toBe(false);
});
});
});
});