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

[Defer Uploads v2] Added support for handling multiple root resource in single asset discovery phase #1728

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
6 changes: 6 additions & 0 deletions packages/core/src/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ export function createPercyServer(percy, port) {
build: percy.testing?.build ?? percy.build,
loglevel: percy.loglevel(),
config: percy.config,
widths: {
// This is always needed even if width is passed
mobile: percy.deviceDetails ? percy.deviceDetails.map((d) => d.width) : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt we send dpr as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't make sense as we are just resizing page and not reloading page so changing dpr wouldn't help

// This will only be used if width is not passed in options
config: percy.config.snapshot.widths
},
success: true,
type: percy.client.tokenType()
}))
Expand Down
14 changes: 14 additions & 0 deletions packages/core/src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ export const configSchema = {
sync: {
type: 'boolean'
},
multiDOM: {
type: 'boolean',
default: false
},
testCase: {
type: 'string'
},
Expand Down Expand Up @@ -291,6 +295,7 @@ export const snapshotSchema = {
domTransformation: { $ref: '/config/snapshot#/properties/domTransformation' },
enableLayout: { $ref: '/config/snapshot#/properties/enableLayout' },
sync: { $ref: '/config/snapshot#/properties/sync' },
multiDOM: { $ref: '/config/snapshot#/properties/multiDOM' },
Copy link
Contributor

Choose a reason for hiding this comment

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

is multiDOM confirmed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesnt sound very straightforward for end user. captureMultipleWidths seems better - it does exactly what it says.

testCase: { $ref: '/config/snapshot#/properties/testCase' },
labels: { $ref: '/config/snapshot#/properties/labels' },
thTestCaseExecutionId: { $ref: '/config/snapshot#/properties/thTestCaseExecutionId' },
Expand Down Expand Up @@ -473,6 +478,15 @@ export const snapshotSchema = {
items: { type: 'string' }
}
}
}, {
type: 'array',
items: {
type: 'object',
properties: {
domSnapshot: { $ref: '/snapshot#/$defs/dom/properties/domSnapshot/oneOf/1' },
width: { $ref: '/config/snapshot#/properties/widths/items' }
}
}
}]
}
},
Expand Down
96 changes: 68 additions & 28 deletions packages/core/src/discovery.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,21 +88,52 @@ function waitForDiscoveryNetworkIdle(page, options) {

// Creates an initial resource map for a snapshot containing serialized DOM
function parseDomResources({ url, domSnapshot }) {
if (!domSnapshot) return new Map();
let isHTML = typeof domSnapshot === 'string';
let { html, resources = [] } = isHTML ? { html: domSnapshot } : domSnapshot;
let rootResource = createRootResource(url, html);
const map = new Map();
if (!domSnapshot) return map;
let allRootResources = new Set();
let allResources = new Set();

if (!Array.isArray(domSnapshot)) {
domSnapshot = [{ domSnapshot }];
Copy link
Contributor

@ninadbstack ninadbstack Sep 19, 2024

Choose a reason for hiding this comment

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

should this be an object array with domSnapshot key i it ? that doesnt seem to match your schema. It should be domSnapshot = [ domSnapshot, ]; as per schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-09-19 at 11 06 58 PM
This is correct. check schema

Copy link
Contributor

@ninadbstack ninadbstack Sep 20, 2024

Choose a reason for hiding this comment

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

Any specific reason to put width and domSnapshot separately ? shouldnt width be part of it ?

Especially considering you are defining it in schema separately but not passing it here on line 97 ? if its optional it would work with domSnapshot as well will keep connected items together

}

for (let snapshot of domSnapshot) {
const dom = snapshot.domSnapshot;
let isHTML = typeof dom === 'string';
let { html, resources = [] } = isHTML ? { html: dom } : dom;
resources.forEach(r => allResources.add(r));
const attrs = snapshot.width ? { widths: [snapshot.width] } : {};
let rootResource = createRootResource(url, html, attrs);
allRootResources.add(rootResource);
}
allRootResources = Array.from(allRootResources);
map.set(allRootResources[0].url, allRootResources);
allResources = Array.from(allResources);

// reduce the array of resources into a keyed map
return resources.reduce((map, { url, content, mimetype }) => {
return allResources.reduce((map, { url, content, mimetype }) => {
ninadbstack marked this conversation as resolved.
Show resolved Hide resolved
// serialized resource contents are base64 encoded
content = Buffer.from(content, mimetype.includes('text') ? 'utf8' : 'base64');
// specify the resource as provided to prevent overwriting during asset discovery
let resource = createResource(url, content, mimetype, { provided: true });
// key the resource by its url and return the map
return map.set(resource.url, resource);
// the initial map is created with at least a root resource
}, new Map([[rootResource.url, rootResource]]));
}, map);
}

function createAndApplyPercyCSS({ percyCSS, roots }) {
let css = createPercyCSSResource(roots[0].url, percyCSS);

// replace root contents and associated properties
roots.forEach(root => {
Object.assign(root, createRootResource(root.url, (
root.content.replace(/(<\/body>)(?!.*\1)/is, (
`<link data-percy-specific-css rel="stylesheet" href="${css.pathname}"/>`
) + '$&'))));
});

return css;
}

// Calls the provided callback with additional resources
Expand All @@ -111,14 +142,14 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) {
resources = [...(resources?.values() ?? [])];

// find any root resource matching the provided dom snapshot
let rootContent = domSnapshot?.html ?? domSnapshot;
let root = resources.find(r => r.content === rootContent);
// since root resources are stored as array
let roots = resources.find(r => Array.isArray(r));

// initialize root resources if needed
if (!root) {
if (!roots) {
let domResources = parseDomResources({ ...snapshot, domSnapshot });
resources = [...domResources.values(), ...resources];
root = resources[0];
roots = resources.find(r => Array.isArray(r));
}

// inject Percy CSS
Expand All @@ -129,16 +160,13 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) {
log.warn('DOM elements found outside </body>, percyCSS might not work');
}

let css = createPercyCSSResource(root.url, snapshot.percyCSS);
ninadbstack marked this conversation as resolved.
Show resolved Hide resolved
resources.push(css);

// replace root contents and associated properties
Object.assign(root, createRootResource(root.url, (
root.content.replace(/(<\/body>)(?!.*\1)/is, (
`<link data-percy-specific-css rel="stylesheet" href="${css.pathname}"/>`
) + '$&'))));
const percyCSSReource = createAndApplyPercyCSS({ percyCSS: snapshot.percyCSS, roots });
resources.push(percyCSSReource);
}

// For multi dom root resources are stored as array
resources = resources.flat();

// include associated snapshot logs matched by meta information
resources.push(createLogResource(logger.query(log => (
log.meta.snapshot?.testCase === snapshot.meta.snapshot.testCase && log.meta.snapshot?.name === snapshot.meta.snapshot.name
Expand Down Expand Up @@ -195,16 +223,19 @@ async function* captureSnapshotResources(page, snapshot, options) {
};

// used to resize the using capture options
let resizePage = width => page.resize({
height: snapshot.minHeight,
deviceScaleFactor,
mobile,
width
});
let resizePage = width => {
page.network.currentWidth = width;
Copy link
Contributor

Choose a reason for hiding this comment

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

Network is not directly related to current width. Dont update in network. Update in intercept instead as intercept needs current width to save/retrieve resources. Network just passes it to intercept in the end

return page.resize({
height: snapshot.minHeight,
deviceScaleFactor,
mobile,
width
});
Comment on lines +227 to +232
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally if we are having too many CDP commands shouldn't we consider creating a wrapper class which will receive parameters and do the things using that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually page class is a wrapper only for running different cdp commands on browser.

};

// navigate to the url
yield resizePage(snapshot.widths[0]);
yield page.goto(snapshot.url, { cookies });
yield page.goto(snapshot.url, { cookies, forceReload: discovery.captureResponsiveAssetsEnabled });

// wait for any specified timeout
if (snapshot.discovery.waitForTimeout && page.enableJavaScript) {
Expand All @@ -228,7 +259,8 @@ async function* captureSnapshotResources(page, snapshot, options) {
// Running before page idle since this will trigger many network calls
// so need to run as early as possible. plus it is just reading urls from dom srcset
// which will be already loaded after navigation complete
if (discovery.captureSrcset) {
// Don't run incase of multiDOM since we are running discovery for all widths so images will get captured in all required widths
if (!snapshot.multiDOM && discovery.captureSrcset) {
ninadbstack marked this conversation as resolved.
Show resolved Hide resolved
await page.insertPercyDom();
yield page.eval('window.PercyDOM.loadAllSrcsetLinks()');
}
Expand All @@ -247,6 +279,7 @@ async function* captureSnapshotResources(page, snapshot, options) {
yield page.evaluate(execute?.beforeResize);
yield waitForDiscoveryNetworkIdle(page, discovery);
yield resizePage(width = widths[i + 1]);
if (snapshot.multiDOM) { yield page.goto(snapshot.url, { cookies, forceReload: true }); }
yield page.evaluate(execute?.afterResize);
}
}
Expand Down Expand Up @@ -365,8 +398,15 @@ export function createDiscoveryQueue(percy) {
disableCache: snapshot.discovery.disableCache,
allowedHostnames: snapshot.discovery.allowedHostnames,
disallowedHostnames: snapshot.discovery.disallowedHostnames,
getResource: u => snapshot.resources.get(u) || cache.get(u),
saveResource: r => { snapshot.resources.set(r.url, r); if (!r.root) { cache.set(r.url, r); } }
getResource: (u, width = null) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this responsible for returning the resource for that width?
If width is not present what are we planning to do, I am seeing we are returning first index are we sure its works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it works. since only root resource is an array and in case if multiDOM is disabled we are storing root resource as a array only with no widths. that case is working fine. for other resources no change in logic

let resource = snapshot.resources.get(u) || cache.get(u);
if (resource && Array.isArray(resource) && resource[0].root) {
const rootResource = resource.find(r => r.widths?.includes(width));
resource = rootResource || resource[0];
}
return resource;
},
saveResource: r => { snapshot.resources.set(r.url, r); cache.set(r.url, r); }
}
});

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/network.js
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ async function sendResponseResource(network, request, session) {
let send = (method, params) => network.send(session, method, params);

try {
let resource = network.intercept.getResource(url);
let resource = network.intercept.getResource(url, network.currentWidth);
Copy link
Contributor

Choose a reason for hiding this comment

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

apart from network.currentWidth is there any way to get any width this looks little weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can use cdp to get current window width but that will be slower operation as compared to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have commented to make it better

network.log.debug(`Handling request: ${url}`, meta);

if (!resource?.root && hostnameMatches(disallowedHostnames, url)) {
Expand Down Expand Up @@ -495,7 +495,7 @@ async function saveResponseResource(network, request) {
}
}

if (resource) {
if (resource && !resource.root) {
network.intercept.saveResource(resource);
}
}
Expand Down
7 changes: 6 additions & 1 deletion packages/core/src/page.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,14 @@ export class Page {
}

// Go to a URL and wait for navigation to occur
async goto(url, { waitUntil = 'load', cookies } = {}) {
async goto(url, { waitUntil = 'load', cookies, forceReload } = {}) {
this.log.debug(`Navigate to: ${url}`, this.meta);

if (forceReload) {
this.log.debug('Navigating to blank page', this.meta);
await this.session.send('Page.navigate', { url: 'about:blank' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need to wait for navigation here ? does it work regardless ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

about:blank is an empty page and does not load any external resources so i don't think we need to wait for it to load. it is instantaneous

Copy link
Contributor

Choose a reason for hiding this comment

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

have we added tests for this ? or ^ is from chromium documentation ?

}

let navigate = async () => {
const userPassedCookie = this.session.browser.cookies;
// set cookies before navigation so we can default the domain to this hostname
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ export function createResource(url, content, mimetype, attrs) {

// Creates a root resource object with an additional `root: true` property. The URL is normalized
// here as a convenience since root resources are usually created outside of asset discovery.
export function createRootResource(url, content) {
return createResource(normalizeURL(url), content, 'text/html', { root: true });
export function createRootResource(url, content, attrs = {}) {
return createResource(normalizeURL(url), content, 'text/html', { ...attrs, root: true });
}

// Creates a Percy CSS resource object.
Expand Down
14 changes: 14 additions & 0 deletions packages/core/test/api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ describe('API Server', () => {
success: true,
loglevel: 'info',
config: PercyConfig.getDefaults(),
widths: { mobile: [], config: PercyConfig.getDefaults().snapshot.widths },
build: {
id: '123',
number: 1,
Expand All @@ -69,6 +70,19 @@ describe('API Server', () => {
});
});

it('should return widths present in config and fetch widths for devices', async () => {
await percy.start();
percy.deviceDetails = [{ width: 390, devicePixelRatio: 2 }];
percy.config = PercyConfig.getDefaults({ snapshot: { widths: [1000] } });

await expectAsync(request('/percy/healthcheck')).toBeResolvedTo(jasmine.objectContaining({
widths: {
mobile: [390],
config: [1000]
}
}));
});

it('can set config options via the /config endpoint', async () => {
let expected = PercyConfig.getDefaults({ snapshot: { widths: [1000] } });
await percy.start();
Expand Down
Loading
Loading