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

feat: added defer uploads for percy-storybook #847

Closed
wants to merge 9 commits into from

Conversation

nilshah98
Copy link
Contributor

@nilshah98 nilshah98 commented Dec 1, 2023

Context-

  • defer-uploads is used for capturing multiple dom snapshots, for same webpage.
  • but in percy-storybook, we do not respect the defer-uploads config
  • this pr, consumes defer-uploads config, and then takes multiple snapshots for same webpage depending on number of widths

Why test cases have changes so much ?

  • Earlier percy-storybook versions were using percy cli version - 1.24.0
  • In this version, the defer-uploads config was not passed successfully from cli -> storybook due to configSchema being outdated in 1.24.0
  • Hence, updated cli-command package to 1.27.4 which fixed this issue and defer-uploads tests started working.
  • But with 1.27.4 we also updated the createTestServer mock service with this particular PR.
  • In summary what above PR does is, it treats /iframe.html and /iframe.html?id=1 as different requests and responds to them differently.
  • Hence, in percy storybook, we first load iframe.html get all stories and then navigate to iframe.html?id={storyId} which is not available anymore.
  • Therefore added additional mocks, which return same response for iframe.html and iframe.html?id={storyId}&viewMode=story route.

Related PRs-

@nilshah98 nilshah98 requested a review from a team as a code owner December 1, 2023 13:10
src/snapshots.js Outdated
options.domSnapshot = domSnapshot;
// validate without logging to prune all other options
PercyConfig.validate(options, '/snapshot/dom');
// validate without logging to prune all other options
Copy link
Contributor

Choose a reason for hiding this comment

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

incorrect comment

src/snapshots.js Outdated
} else {
log.debug(`Loading story: ${options.name}`);
// when not dry-running and javascript is not enabled, capture the story dom
yield page.eval(evalSetCurrentStory, { id, args, globals, queryParams });
/* istanbul ignore next: tested, but coverage is stripped */
let { dom, domSnapshot = dom } = yield page.snapshot(options);
options.domSnapshot = domSnapshot;
for (let i = 0; i < percy.config.snapshot.widths.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will increase User's CI time considerably, we've seen few customers using on packed CI with multiple widhts.

we should ideally ship this behind feature, or have it as accepted bug.

Copy link
Contributor

@ninadbstack ninadbstack Dec 1, 2023

Choose a reason for hiding this comment

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

There are also issues with logging, we are only doing an alpha release for testing it with client tentatively and we will need to fix some things [ logging, multiple widths etc ] before we can add this to stable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, we will rely on the customer's percy config to provide defer-uploads

Copy link
Contributor

@ninadbstack ninadbstack left a comment

Choose a reason for hiding this comment

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

Currently it also means that we dont have tests with defer uploads true. But okay for alpha poc

Copy link

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the 🍞 stale Closed due to inactivity label Dec 19, 2023
Copy link

github-actions bot commented Jan 2, 2024

This PR was closed because it has been stalled for 28 days with no activity.

@github-actions github-actions bot closed this Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍞 stale Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants