Skip to content

✨ Accept additional snapshot execute options for various states #514

Merged
wwilsman merged 2 commits intomasterfrom
ww/more-snapshot-execute-options
Aug 20, 2021
Merged

✨ Accept additional snapshot execute options for various states #514
wwilsman merged 2 commits intomasterfrom
ww/more-snapshot-execute-options

Conversation

@wwilsman
Copy link
Copy Markdown
Contributor

What is this?

This PR adds the ability to specify additional execute options to run scripts at specific states during the snapshot process. By default, the original execute function would only be evaluated right before a snapshot is taken. Now, there are additional options for afterNavigation, beforeResize, afterResize, and beforeSnapshot. If execute is provided as a function without these additional options, it is treated as a shorthand for execute.beforeSnapshot.

Thanks to @cancan101 for the idea! With these new options, it makes it easier for people to handle lazy-loaded images before the page gets resized. Closes #513

After navigation, before and after resizing, and before the snapshot is taken
@wwilsman wwilsman added the ✨ enhancement New feature or request label Aug 20, 2021
@wwilsman wwilsman requested a review from Robdel12 August 20, 2021 19:41
Copy link
Copy Markdown
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁

@wwilsman wwilsman merged commit 5d09fe9 into master Aug 20, 2021
@wwilsman wwilsman deleted the ww/more-snapshot-execute-options branch August 20, 2021 20:33
@cancan101
Copy link
Copy Markdown

Possible to get a release with this?

@wwilsman
Copy link
Copy Markdown
Contributor Author

We were going to release this today, but we decided to hold off until Monday since some other stuff will be going out with it and we want to be available if an unforeseen issue comes up.

@cancan101
Copy link
Copy Markdown

Giving this a shot and getting the following error:

[percy:core:page] Evaluate JavaScript (4ms)
[percy:core] Encountered an error taking snapshot: About (1ms)
[percy:core] Error: The provided function is not serializable

Not sure I updated the snapshots.yml file correctly (is it still possible to use a yaml file)?

- name: About
  url: http://localhost:3000/about
  waitForTimeout: 100
  execute:
    afterNavigation: 'function sleep(ms) {return new Promise(resolve => setTimeout(resolve, ms));} window.scrollTo({ top: document.body.scrollHeight, behavior: "smooth" }); await sleep(1000); window.scrollTo(0,0); await sleep(100);'
  widths:
    - 375

@wwilsman
Copy link
Copy Markdown
Contributor Author

Looks like a bug 🙃

Will fix and release a new version shortly 👍

@patricknelson
Copy link
Copy Markdown

Documentation challenge, maybe, but: Wouldn't the beforeResize and afterResize configuration directives only be relevant server-side (Percy's end) and not at snapshot time (i.e. in local CLI)? Or, does the introduction of this configuration directive trigger the CLI to then capture a snapshot of the DOM for every one of the configured widths? Otherwise, how is it going to fire beforeResize and afterResize when it doesn't run RS remotely?

@patricknelson
Copy link
Copy Markdown

p.s. The reason why this is important for me is because unfortunately I have several places on the 30-40 or so pages that I'm testing which need to re-run JavaScript in order to properly render for the current breakpoint. However, architecturally, it looks like Percy:

  1. Takes a single DOM snapshot locally (essentially capturing the HTML in its current state, the inline styles, CSS/JS/image assets and etc)
  2. Iterates through each width defined and then takes a screenshot for each browser

This is great, but one minor problem with this setup is that it results in me needing to manually define repetitive/duplicate entries over and over again when I could just have a single afterResize, for example:

  - name: "library-quotes-mobile"
    url: "http://example.com/content-section-library/quotes/"
    widths: [375] # IMPORTANT: Separate captures are necessary to validate desktop/mobile due to the JS required to render each layout separately.
  - name: "library-quotes-desktop"
    url: "http://example.com/content-section-library/quotes/"
    widths: [1280] # See important note above.

But it would be simpler (and far easier to maintain) if I could just do the following:

  - name: "library-quotes"
    url: "http://example.com/content-section-library/quotes/"
    execute:
      afterResize: "Example.Layout.triggerResize()"

Note that now in this case I'm not repeating my widths, which is great since it's then far easier to centrally toggle/reconfigure/etc in my root .percy.yml configuration.

(Sorry for all the notifications 😬)

@cancan101
Copy link
Copy Markdown

I'm not sure I understand the need to break up the entries. This is what we do:

- name: "library-quotes"
  url: "http://example.com/content-section-library/quotes/"
  execute:
    afterNavigation: |
      function sleep(ms) {
        return new Promise(resolve => setTimeout(resolve, ms));
      }
      let sections = Math.ceil(document.body.scrollHeight/window.innerHeight);
      for(var i = 1; i <=sections; i++){
        window.scrollTo({top: (document.body.scrollHeight/sections)*i, behavior: "smooth"});
        await sleep(500);
        // in case the height changes
        sections = Math.ceil(document.body.scrollHeight/window.innerHeight);
      }
      window.scrollTo(0, 0);
      await sleep(100);
  widths:
    - 375
    - 768
    - 1200
    - 1500

@patricknelson
Copy link
Copy Markdown

I have components already in the page (not React, but jQuery, it’s an old site…) that have corresponding JavaScript code that is responsible for helping to configure/layout those components depending on the current page width (breakpoint). Re-read what I said above (just so you’re familiar with my concern about the problem domain).

I’m new to this, but basically, Percy has two contexts: 1.) The CLI that launches a custom Chromium instance that scrapes HTML/JavaScript/CSS/etc and 2.) The cloud-based rendering browsers. My issue is that the jQuery component must run JavaScript in order to reposition, rearrange, move (extract, append, relocate, etc) various DOM elements when it notices a resize event in the browser. From what I understand of how Percy’s architecture works, this can only be done (reliably) in context 1, in the CLI running the local Chromium browser doing the scraping.

That’s why I asked the question above. Because afterResize doesn’t seem to be working for me at all when I try to call an internal JS function. This suggests to me it doesn’t run locally but maybe in the cloud? I have no clue… this seems like a documentation gap, maybe. Right or am I totally off base with how these contexts work @wwilsman? 😕

@cancan101
Copy link
Copy Markdown

Ah ok, I think I see what you are saying. You need to run the JS in the rendering context (ie for each of the bowsers + widths being tested on) since you actually want to change the DOM as width changes (rather than relying on CSS, etc). Yes, that I don't know how / if Percy handles. I would think if they don't currently handle, and this is a common use case, then some sort of syntactic sugar could be introduced to get the same effect that you are looking for where you have to split up the entries in the yaml file.

@patricknelson
Copy link
Copy Markdown

patricknelson commented Mar 30, 2022

Right, so that’s fine. I thankfully have ways around it (just a duplicate entry). Not a huge deal. And would be cool to have a more succinct syntax.

But still, it makes me curious… if configs like execute.beforeResize don’t run locally, where do they run? Since there are multiple widths, and normally changing between them triggers resize, then is this just the one resize that fires on initial load, I suppose? I are confused. 😕

EDIT: Or it could be that it does run in the remote Percy browser context, it just doesn’t have access to the app’s bundled JS, it’s just raw JS that runs in situ (even if it modifies DOM) and can only run in isolation before/after each resize.

@cancan101
Copy link
Copy Markdown

beforeResize runs in context 1. e.g. when percy is capturing all of the needed assets that then get shipped off to the back end rendering (context 2)

repasting my yaml as I left off a key:

- name: "library-quotes"
  url: "http://example.com/content-section-library/quotes/"
  execute:
    afterNavigation: |
      function sleep(ms) {
        return new Promise(resolve => setTimeout(resolve, ms));
      }
      let sections = Math.ceil(document.body.scrollHeight/window.innerHeight);
      for(var i = 1; i <=sections; i++){
        window.scrollTo({top: (document.body.scrollHeight/sections)*i, behavior: "smooth"});
        await sleep(500);
        // in case the height changes
        sections = Math.ceil(document.body.scrollHeight/window.innerHeight);
      }
      window.scrollTo(0, 0);
      await sleep(100);
    afterResize: |
      function sleep(ms) {
        return new Promise(resolve => setTimeout(resolve, ms));
      }
      let sections = Math.ceil(document.body.scrollHeight/window.innerHeight);
      for(var i = 1; i <=sections; i++){
        window.scrollTo({top: (document.body.scrollHeight/sections)*i, behavior: "smooth"});
        await sleep(500);
        // in case the height changes
        sections = Math.ceil(document.body.scrollHeight/window.innerHeight);
      }
      window.scrollTo(0, 0);
      await sleep(100);
  widths:
    - 375
    - 768
    - 1200
    - 1500

@patricknelson
Copy link
Copy Markdown

And this only runs once at snapshot time, or does it run for each width (and thus behind the scenes technically sending multiple DOM snapshots bundled into the same alias, just loaded under each width)?

@patricknelson
Copy link
Copy Markdown

patricknelson commented Mar 31, 2022

Also, just had a chance to try this out @wwilsman and I think there's a bug where execute.afterNavigation works but not when under additionalSnapshots. Using @cancan101's code as an example

This works:

  - name: "name"
    url: http://localhost:3000/about
    waitForSelector: ".vqa-ready"
    execute:
      afterNavigation: "$('body').addClass('vqa-ready');"

But this does not:

  - name: "name"
    url: http://localhost:3000/about
    additionalSnapshots:
      - suffix: "-toggled"
        waitForSelector: ".vqa-ready"
        execute:
          afterNavigation: "$('body').addClass('vqa-ready');"

@wwilsman
Copy link
Copy Markdown
Contributor Author

Hey @patricknelson! I'll try to clear up how the snapshot command executes those functions for you!

  1. When a snapshot is being captured, it navigates to the snapshot URL and then calls the execute.afterNavigation function: https://github.com/percy/cli/blob/v1.0.0-beta.76/packages/core/src/snapshot.js#L214-L216

  2. Afterwards, widths are iterated over to capture responsive assets (the snapshot DOM has not been captured yet). Around each resize, execute.beforeResize and execute.afterResize are called: https://github.com/percy/cli/blob/v1.0.0-beta.76/packages/core/src/snapshot.js#L218-L224

  3. It is at this point, after navigation and resizing, when additional snapshots are processed. The purpose of these additional snapshots is to capture multiple states of the same page (clicking a button, displaying a modal, etc.). Right before the moment the DOM is captured for a snapshot, the execute.beforeSnapshot function is called (or just execute when providing a single function): https://github.com/percy/cli/blob/v1.0.0-beta.76/packages/core/src/page.js#L194-L197

For additional snapshots, the page has already been navigated to and responsive assets have already been captured. As such, the navigation and resize functions are not accepted under additional snapshots. In fact, the only accepted function for additionalSnapshots is just execute: https://github.com/percy/cli/blob/v1.0.0-beta.76/packages/core/src/config.js#L183

Also to note, none of these execute options are sent to Percy's renderers. These functions are meant to aid in asset discovery. Once the DOM is captured, it and any captured assets are sent with basic snapshot options: https://github.com/percy/cli/blob/v1.0.0-beta.76/packages/client/src/client.js#L306-L309

Also also, you can utilize YAML anchors and references to avoid duplication. You'll just have to adjust your yaml to have a top-level snapshots: key with all your nested snapshots there. Then you can use any other top level keys as anchors and references. Or if you prefer JavaScript, you could also use a .js file instead of a .yml file. Just export an array of snapshots or a function that returns an array of snapshots.


With all that said, the snapshot command is not meant to replace a complete test suite. So if you have a complex site or app, I would highly recommend to set up a testing framework and integrate a Percy SDK with that (Puppeteer, Playwright, or Cypress are quick and easy to set up). Here are our currently supported SDKs: https://docs.percy.io/docs/sdks#end-to-end-testing-frameworks

@patricknelson
Copy link
Copy Markdown

Thank you @wwilsman! This breakdown is extremely helpful. The very simplicity & approachability of the YAML config is precisely what makes it so appealing for our large site. It's just those darned edge cases that throw a wrench into things, requiring a deeper nuts-and-bolts understanding of the complete YAML config schema as well as what each config param actually does. So, thank you for this explanation.

What is the purpose of waitForSelector under additionalSnapshots? The reason I ask is because while it does work if a selector was dynamically added in the root execute, it never sees the .vqa-ready selector if it is actually added in the adjacent execute like below.

  - name: "test"
    url: "http://example.com/test/"
    additionalSnapshots:
      - suffix: "-example"
        execute: "$('body').addClass('vqa-ready')"
        waitForSelector: ".vqa-ready"

image

@wwilsman
Copy link
Copy Markdown
Contributor Author

wwilsman commented Mar 31, 2022

The waitForSelector option is actually awaited on right before execute is called: https://github.com/percy/cli/blob/v1.0.0-beta.76/packages/core/src/page.js#L183

We definitely want to and need to document this snapshot command more. For example, you can actually wait on things right inside execute with a provided helper:

# The `waitFor` helper is available for all `execute` functions.
# It resolves when the callback returns a truthy value, or rejects after the timeout
execute: |
  await waitFor(() => $('body').hasClass('vqa-ready'), 1000);
  // ... do other stuff after waiting

Here is the function signature for that execute helper, which also accepts a couple other options as the second argument: https://github.com/percy/cli/blob/v1.0.0-beta.76/packages/core/src/utils.js#L99

@cancan101
Copy link
Copy Markdown

On that note an example of how to use waitFor to sleep would be cool. We have your own sleep implementation that we use between scrolls.

@patricknelson
Copy link
Copy Markdown

patricknelson commented Apr 1, 2022

Hmm, interesting.. ok, got it!

Basically: If you ever need to wait for a selector to be present that would be caused by execute, you must perform the waiting inside your execute and not by using the waitForSelector, given those two configurations run synchronously and out of order. That's not immediately intuitive to me as a user, but changing it now would probably break a lot of tests (the ole' "Here be dragons") so I will not be venturing into that cave. 😄

That said: Are those core library utilities you linked to above (including waitFor()) exposed for our use as end-users, or are those intended only for internal use within the library itself? If the latter, I'd probably rather avoid it for now since, presumably, relying on that could lead to buggy behavior if it ever changes.

@wwilsman
Copy link
Copy Markdown
Contributor Author

wwilsman commented Apr 1, 2022

@patricknelson you can count on waitFor sticking around and not changing. The only other utility exposed for now is generatePromise, but that one may change in the near future.

@cancan101 the waitFor util was made to await on states (waitForSelector actually uses it under the hood). In your case, between scrolls you could wait for the window.scrollY state to be equal to a specific value.

// wait until window has scrolled down to the section
// note that without a timeout, it will continue to poll until true
await waitFor(() => window.scrollY === sectionTop)

@cancan101
Copy link
Copy Markdown

I actually do want to wait for a specific amount of time to allow lazy loaded images, etc to load. I would also be worried with using the example code as you presented in case the height of the page changes as it scroll leading the equality comparison never evaluating as true.

Anyway, I would put in a vote to inject a time delay function to the namespace. ;-)

@wwilsman
Copy link
Copy Markdown
Contributor Author

wwilsman commented Apr 1, 2022

@cancan101 😁 #857

@patricknelson
Copy link
Copy Markdown

@wwilsman your comments above are so helpful, it'd be awesome if the public facing site also had these things documented as well (both the technical breakdown you made here as well as the official support for waitFor and generatePromise). I know that could get tricky to make sure you don't water down (or over complicate) the current easy-to-read documentation already on the site.

samarsault pushed a commit that referenced this pull request Mar 3, 2023
Bumps [tsd](https://github.com/SamVerschueren/tsd) from 0.20.0 to 0.21.0.
- [Release notes](https://github.com/SamVerschueren/tsd/releases)
- [Commits](tsdjs/tsd@v0.20.0...v0.21.0)

---
updated-dependencies:
- dependency-name: tsd
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ability to Execute Javascript between Resizes on Percy CLI Snapshot

4 participants