search

percy / percy-capybara

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

Enabling javascript causes images to double in height #51

Closed rsimha closed 3 years ago

rsimha commented 6 years ago

We're using percy/capybara to generate snapshots in our visual tests for the AMP project. We recently switched from PhantomJS to Chrome / Selenium. Today, I noticed that our snapshots were missing certain portions due to the fact that JavaScript wasn't enabled on the Percy side. (The content showed up on my local machine when the page was first rendered prior to snapshotting, but was missing in the final snapshots because Percy doesn't enable JavaScript by default.)

When I repeated the snapshots with JavaScript enabled (using Percy::Capybara.snapshot(page, name: name, enable_javascript: true)), I noticed that the mere act of enabling JavaScript is causing all our image assets to double in height in the snapshots. (They appear normal when rendered locally, prior to snapshotting.) This is mysterious behavior and I'm not sure what we can do to prevent this. See the snapshots in https://percy.io/ampproject/amphtml/builds/538714 for example. The baselines do not have JavaScript enabled, while the new snapshots do.

FYI, here is how we are setting up Capybara to use Chrome instead of PhantomJS.

Left: with JS, Right: without JS

rsimha commented 6 years ago

/to @fotinakis @timhaines

timhaines commented 6 years ago

Hi @rsimha. In one snapshot, the images sources are img/hero@1x.jpg and img/sea@1x.jpg and in the other they're img/hero@2x.jpg and img/sea@2x.jpg

Do you have some JS in place that's switching the img source?

It looks like the css you have setup maintains the image height, but fixes the width?

Is this enough info for you to resolve this?

rsimha commented 6 years ago

@timhaines I didn't have visibility into that behavior, so I wasn't aware that the img source was at play. I'll investigate on our side. Thanks!

rsimha commented 6 years ago

@timhaines Unfortunately, after investigating, this appears to be a bug. I re-ran the visual tests locally, and when img/hero@2x.jpg is rendered locally, it appears in the correct aspect ratio. However, the Percy snapshots are displaying the wrong aspect ration (way taller than it is supposed to be).

The original images are here: https://github.com/ampproject/amphtml/tree/master/examples/visual-tests/article-access.amp/img

Any ideas?

rsimha commented 6 years ago

A more illustrative example of what's going wrong is here: https://percy.io/ampproject/amphtml/builds/538856/view/25118149/375?mode=head&snapshot=25118149

Scroll down to see that the image that's supposed to have a 1x1 aspect ratio is being rendered with double the height. When I ran this test locally with Percy / Capybara / Chrome, I noticed that the images were showing up with the correct aspect ratio.

rsimha commented 6 years ago

@timhaines I did a visual diff with another test page, which does more stuff with JS to see if this is an overall JS problem with the Percy backend. This test page is designed to display the first paragraph of an article, with a "Login to read more" message to indicate that the user hasn't subscribed to the service.

Here is what I'm seeing on my machine when Percy / Capybara / Chrome are in action. Notice that the image is showing in the correct aspect ratio, and there are no JS errors. Also notice that only the first paragraph of the article is displayed, followed by the "Login to read more" message, as expected.

However, when this same page is rendered by the Percy backend, here's the snapshot that is generated (I've cropped just the top portion). Notice that the image is double the height, the entire article is displayed, and the "Login to read more" message doesn't show up, but instead, we see the "Oops, something broke" message. Any idea what's going on here? (link to snapshot)

rsimha commented 6 years ago

@fotinakis @timhaines We're seeing the same issue with our real-world page visual tests that use the Python library. See https://percy.io/ampproject/danielrozenberg/builds/539438

Since this is breaking all our tests right now, would it be possible to prioritize the investigation of this issue? Note that all these pages render perfectly fine on a local Chrome browser, so I'm quite sure this has something to do with how the Percy backend renders AMP pages from DOM snapshots with JS enabled.

/cc @danielrozenberg @cramforce

timhaines commented 6 years ago

@rsimha We will spend some time looking into this. In the interim, are builds with javascript disabled still rendering the images correctly for you?

danielrozenberg commented 6 years ago

I think we might be using the API in an unexpected way. I used a Selenium driver to load the page on a local server, then I send the snapshot with enable_javascript=True. What I think is happening is that it takes the DOM snapshot, then on your server side since js is enable it re-does all the loading behaviours of AMP, which screw up things in many ways.

I tried a different approach right now where I skip the local server and Selenium driver, and directly send the raw HTML of each tested page with js enabled. This seems to produce the expected results for me (but I'm still investigating whether this is the approach we want to take)

fotinakis commented 6 years ago

@rsimha that is an odd one, we can help you look into this. Digging through the code briefly, we recommend removing enable_javascript: true in the snapshot call because the JS is already being executed locally when you're running through in Chrome headless and generating the DOM snapshot. — ie. you shouldn't mix Capybara with javascript enabled and Percy with javascript enabled, it should be either or.

What content is missing from the pages when you don't have javascript enabled? If you run the tests without --headless or use capybara's save_and_open_screenshot locally (https://github.com/teamcapybara/capybara#debugging), do you see the content there?

rsimha commented 6 years ago

@fotinakis thanks for pointing out how enabling JS for both Capybara and Percy isn't the right thing to do. That part now makes sense to me.

Re: missing content, first off, I'm not using --headless to generate DOM snapshots (I see a real Chrome window). Yet, we're missing these items:

rsimha commented 6 years ago

In the interim, are builds with javascript disabled still rendering the images correctly for you?

@timhaines builds with JS disabled are rendering images in the correct size, but they're missing all kinds of other content.

For example, see the build at https://percy.io/ampproject/amphtml/builds/539721 (Original: JS enabled on Percy, New: JS disabled on Percy)

timhaines commented 6 years ago

@rsimha @fotinakis I have a theory about why image sizes are rendered at double height when JS is enabled. Let me know what you think.

You're running your tests in an environment that has denser 2x displays, or at least the JS running on your client thinks so, and as a result, it's switching your 1x images to 2x. The same DOM (mostly) as before is being uploaded, but the image paths have been switched to the 2x images.

Our rendering environment is 1x, and you don't have any height constraints on the img tags, so it's rendering the images at full height of the 2x image. However, the images are constrained by width, which is why we're seeing them squished.

Does that sound like a plausible explanation for what we're seeing?

One way to test this would be to get a non-retina device, open one of the example pages, and switch the img source for the 2x version. I think we'd see the same result as what's being rendered in Percy.

@rsimha Do you have a mechanism for making sure 1x images are selected instead of 2x?

timhaines commented 6 years ago

@danielrozenberg @cramforce cc'ing you both on that last comment ^ because I think @rsimha may have gone on vacation, and it sounded like it was somewhat urgent to resolve.

danielrozenberg commented 6 years ago

@timhaines, I believe @rsimha temporarily worked around this issue by merging ampproject/amphtml#13589. He'll be back on Monday and can continue the conversation then. Thanks for taking care of this!

rsimha commented 6 years ago

@timhaines Thanks for the analysis. I've attempted to force Chrome to render locally at a 1x pixel ratio while creating the DOM snapshot in https://github.com/ampproject/amphtml/pull/13694. This didn't seem to work when I initiated the Percy build from my retina macbook. I'm keeping an eye on whether anything changes when the build is initiated from Travis CI.

Meanwhile let me know if you think there's anything wrong with how I'm forcing 1x rendering in https://github.com/ampproject/amphtml/pull/13694.

rsimha commented 6 years ago

@timhaines I just confirmed that I am correctly forcing the pixel ratio to 1x in https://github.com/ampproject/amphtml/pull/13694, and that Chrome is indeed rendering the page locally with the 1x image.

Yet, the Percy renders contain images that are double the height. See https://percy.io/ampproject/amphtml/builds/549815

Not sure what else I can try here.

timhaines commented 6 years ago

Hi @rsimha,

I've looked at the DOM of the uploaded snapshot too, and they specify the 1x image in the source. This is very odd.

Will look deeper into what's happening here to come up with some new ideas.

I don't think this will solve it, but could you try updating that PR you made to not enable javascript on Percy's servers please? I'd be curious to see if that has any impact.

rsimha commented 6 years ago

@timhaines Sounds good, thanks!

The impact of not enabling JS was that several AMP features did not work. See https://github.com/percy/percy-capybara/issues/51#issuecomment-367470936 and https://github.com/percy/percy-capybara/issues/51#issuecomment-367482853 for a partial list of things that are broken.

Edit: I changed my local branch to not enable JS while enforcing 1x rendering, and the AMP features continue to remain broken in the Percy renders. For example, see the "oops something broke" message in https://percy.io/ampproject/amphtml/builds/550166?snapshot=26016836.

timhaines commented 6 years ago

@rsimha Was this build done with javascript off on the client and server? I see the images are normal height: https://percy.io/ampproject/amphtml/builds/550403

rsimha commented 6 years ago

@timhaines Yes, https://percy.io/ampproject/amphtml/builds/550403 was done with JS off. (I confirmed by looking at the message from the head commit in the build info.)

timhaines commented 6 years ago

@rsimha I think I'm getting closer to understanding what's going on here. This is the DOM we're receiving for the snapshot that renders the image with the double height: https://gist.github.com/timhaines/d0206c39450a312c0082d0327fc7b658#file-amp-double-height-html

If you look towards the end of line 283, and at line 284, you'll see it already has a i-amphtml-sizer and img inserted, inside the amp-img element.

If you save this html, and open it in Google Chrome (mirroring what our server is doing), you'll see that a second i-amphtml-sizer and img get inserted in the amp-img element, and the browser allocates twice as much height for the image, due to the two i-amphtml-sizers I think.

However, if I clone your repo, run gulp, and look at the example page, the amp-img doesn't contain either i-amphtml-sizer or an img.

Does this help you understand what's going on, and maybe present you with a solution?

Also, I'm wondering if you've tried disabling javascript execution in your local tests, but having it enabled in Percy snapshot command you run? I'm hoping that might send up the amp-img element without it having been prehydrated?

timhaines commented 6 years ago

@rsimha @danielrozenberg further, when you run gulp and look at the examples locally, if you insert a second i-amphtml-sizer in the amp-img tag, mimic'ing what's happening on Percy's servers, you will see the double height images in your local environment.

rsimha commented 6 years ago

@timhaines Thanks for the detailed analysis. I'll investigate the double i-amphtml-sizer behavior you pointed out and follow up once I know more.

@danielrozenberg too brought up the idea of disabling JS for local rendering, while enabling it on the Percy side. However, we build and serve the AMP runtime locally, and so I'd imagine disabling JS locally will prevent the locally built runtime from being used, which might defeat the purpose of the tests. In any case, I'll try this and let you know what I find out. Thanks again.

rsimha commented 6 years ago

@timhaines I just looked at the raw DOM snapshot you linked to in https://github.com/percy/percy-capybara/issues/51#issuecomment-369065750, and noticed this line, which suggests that the page is pulling the AMP runtime from https://cdn.ampproject.org/v0.js. If this is true, I fear that we're not properly testing the locally built AMP runtime, either with JS disabled (since content is missing), or with JS enabled (since the page will start pulling the production runtime when Percy is rendering it).

Is this founded in reality, or am I not correctly understanding how Percy works?

timhaines commented 6 years ago

@rsimha That script tag is present in the DOM you sent us for that snapshot. If you have javascript enabled on the Percy side, that script will get loaded.

How would you expect the locally built AMP runtime to be injected into the page? Would something be swapping it out?

rsimha commented 6 years ago

Yeah, the locally built AMP runtime gets swapped in by the localhost server when the page is loaded and rendered on the machine that's running the build (either a developer's computer or a Travis CI VM). If one were to take a screenshot at this time, the page would look exactly as desired. We want to compare this with how the page was rendered during past builds with previous versions of the AMP runtime (from master). Could it be that we're using the wrong Percy library?

timhaines commented 6 years ago

Hi @rsimha

I've spent a little more time digging into the script tag issue specifically.

If I clone your project, and run gulp or gulp serve I see the local js. However, if I run gulp visual-diff and open http://localhost:8000/examples/article.amp.html, then I see https://cdn.ampproject.org/v0.js. So, your replacement is not working as you intended when you run gulp visual-diff.

Can you reproduce what I've mentioned here? I think that will help you get to a point where you can troubleshoot why the runtime isn't getting swapped in correctly?

I don't think the problems you're seeing are related to the particular Percy library you're using. This same issue would be occurring if you switched to our webdriver client for example. I suspect the image height issue would too.

I'd like to help you quickly to get to a place where all of these issues are ironed out. Will be in touch further about this generally, probably over email. I'm happy to keep chatting about these two specific issues on GitHub here though.

timhaines commented 6 years ago

@rsimha @danielrozenberg I've tried reproducing the result I was getting here, and I haven't been able to (on master branch, locally). Today gulp visual-diff appears to be replacing https://cdn.ampproject.org/v0.js as expected.

I've inspected the source/DOM that Percy received running this locally, and it does include /dist/amp.js rather than https://cdn.ampproject.org/v0.js. Soooo. I wonder what the conditions/cause are for the replacement to sometimes not happen.

BTW, we released a new feature last week, that allows you to download the DOM that was processed for any snapshot in Percy. It's in the snapshot dropdown next to the width selector.

image