percy / percy-capybara

Visual testing for Capybara with Percy.
https://docs.percy.io/docs/capybara
MIT License
45 stars 23 forks source link

[WIP] Fix iframe directory loading bug #59

Closed faun closed 4 years ago

faun commented 5 years ago

Fix for iframe src as directory discovered in this conversation

Clubhouse card

benalavi commented 5 years ago

Hey @faun, after some digging I believe the issue we're having is that the FilesytemLoader will not discover dynamic iFrames resources.

As far as I can tell in the FilesystemLoader tests it discovers the iFrame resources because they exist statically in the directory structure. So when it later goes to render the page the resources exist in the expected location.

In our case we have an iFrame like:

<iframe name="main" src="/dashboard/" cd_frame_id_="0274b429daf1278235a49a96fe41bd5f"></iframe>

(cd_iframe_id_ is an artifact of testing w/ chromedriver, but I left it in case it was causing any issues). With PERCY_DEBUG turned on /dashboard/ is never discovered as a resource, and I believe that is why nothing is being rendered.

I'm pretty sure the actual cause of the issue is that FilesystemLoader never calls iframes_resources, it just assumes that the iframe resources have already been discovered statically. In fact if I just directly change https://github.com/percy/percy-capybara/blob/v3.1.0/lib/percy/capybara/loaders/filesystem_loader.rb#L27 to:

def snapshot_resources
  [root_html_resource] + iframes_resources
end

it fixes the problem and renders the dynamic iframe content, as you can see here: https://percy.io/openplay/op-music-dev/builds/1082378

I think the spec would be to have the FilesystemLoader discover a resource that does not exist in assets_dir but is referenced by an iframe src tag inside of the root page HTML.

Robdel12 commented 5 years ago

Can this be closed (seeing how v4 doesn't use loaders anymore)?

benalavi commented 5 years ago

Oh great! We’ve been pinned to this fix so far but I will try upgrading and get back today.

Robdel12 commented 5 years ago

I'll hold out on closing until we know this fixes your issue 😃

benalavi commented 5 years ago

@Robdel12 We ran into a handful of issues upgrading Percy so I assume we are an outlier as far as what or how we are testing, but as far as we've been able to tell so far it appears that dynamically loaded iframes do not work with the new version.

Notably any tests that load an iframe get this sort of debug output:

[percy] processing response: http://srg.localhost.openplaymusic.com/dashboard/ for width: 1920
[percy] Skipping [disallowed_response_status_302] [1920 px]: http://srg.localhost.openplaymusic.com/dashboard/

based on these lines from the docs:

We skip redirected assets (like redirecting to a CDN on request. We cannot capture authenticated assets.

I assume what is happening is that the agent is considering the iframe source an asset and fetching it separately rather than getting the HTML content from capybara (or wherever it gets the loaded page content) and then is failing to get the asset because it requires authentication (the 302 redirect response is the agent attempting to request it separately I believe).

So for now I think we are still pinned to this fix and the older version. Is there continuing support for the older version?

For the record the other issue we encountered is that we have a multi-tenant system with subdomain routing where assets are served from a static subdomain (rather than changing with each tenant) to emulate a CDN (i.e. as close to production as possible). The assets are still served locally, but the agent skips them because of the different domain. We worked around it by changing some code to serve assets from each tenant domain in CI.

faun commented 4 years ago

Closing this as stale. We have support for multiple asset hosts in percy-agent at this point, so the listed blockers should be moot.