Pre cropped screenshots #177

trotzig · 2017-01-04T14:19:39Z

No description provided.

This will help people with old firefoxes when we switch over to geckodriver. For people who use other drivers, we bypass the check (you're on your own...).

For a long while now, we've been screenshotting the whole page, then cropping it to only the relevant box on the backend. This is bad for performance, and we believe that the most impactful thing we can do for happo performance right now is to make sure that screenshots are pre-cropped. I started looking at the webdriver spec, and noticed that it has a section about screenshotting an element [1]. Selenium doesn't mention this in docs, but you can use this method. driver.findElement(By.id('foobar')).then((element) => { element.takeScreenshot().then((screenshot) => { // do something }); }); When testing this out, it didn't work in firefox (using the firefox driver). It didn't work in Chrome either. Nor in Safari. It turns out that the only two webdrivers supporting this method is Edge (since a while back) and geckodriver (recently). Performance is definitely improved. On the brigade codebase, a full run used to take roughly 2 min 45 seconds. After this commit, we're down to less than 2 minutes! The larger the viewport, the bigger the win. We run most our examples on 640x888 and 320x444 viewports, where it looks like we're not really gaining much. But the larger viewports have a noticeable perf improvement. [1]: https://www.w3.org/TR/webdriver/#take-element-screenshot

When you call driver.close(), geckodriver will shut down properly. But you're left with a running firefox window. It turns out that this is a known bug, see https://bugzilla.mozilla.org/show_bug.cgi?id=1310992 mozilla/geckodriver#285 We work around that by manually killing any lingering firefox process on close.

This folder contains generated content, we don't want to import it directly.

Changing the cropping behavior is a breaking change, both because other drivers (e.g. chromedriver) won't work out of the box. But also because people are going to have to upgrade firefox as part of this change.

This will hopefully fix #176 through allowing the default uploader (S3) to be overridden in configuration.

trotzig · 2017-01-04T14:20:47Z

@lencioni: Can you have a look at these changes if you have a moment? I still have test failures to fix which I won't have time to do until later during the week. So no rush.

trotzig · 2017-01-04T14:27:42Z

I also need to update the README a little I believe.

lencioni · 2017-01-04T18:01:39Z

src/server/checkBrowserVersion.js

+        const match = stdout.match(FIREFOX_VERSION_MATCHER);
+        if (!match) {
+          reject(new Error(`Failed to parse Firefox version string "${stdout}"`));
+        } else if (parseFloat(match[1]) < MINIMUM_FIREFOX_VERSION) {


I am currently running Firefox 47.0.1. I think we should probably use something that can compare semver numbers to each other instead of just parseFloat.

We can do that when we know we need to, right? For now, this should be enough I think.

lencioni · 2017-01-04T18:04:54Z

src/server/checkBrowserVersion.js

+const config = require('./config');
+
+const MINIMUM_FIREFOX_VERSION = 50.0;
+const FIREFOX_VERSION_MATCHER = /Mozilla Firefox ([0-9.]+)/;


Some Firefox versions end in esr (e.g. 45.6.0esr). It would be nice if the error message we generate includes this detail for maximum clarity.

It looks like we do, don't we?

reject(new Error(`Failed to parse Firefox version string "${stdout}"`));

Oh I was thinking of this line:

`You are using ${match[1]}`));

Ah - of course. yagni?

As it is written, people will potentially see a misleading error message, which seems like it should be fixed.

Alternatively, you could make the error message less useful by not including the current version. That would be a strict improvement over giving potentially incorrect information.

I made it print the full string instead: e16e311

lencioni · 2017-01-04T18:07:01Z

src/HappoRunner.js

@@ -170,6 +170,15 @@ window.happo = {
            width,
          } = getFullRect(rootNodes);

+          const overlay = document.createElement('div');


It might be nice to have a comment here explaining why we are creating an overlay. Or perhaps a more descriptive variable name than overlay. Maybe screenshotOverlay or screenshotBox?

Good call. 90d68b5

lencioni · 2017-01-04T18:07:53Z

src/HappoRunner.js

+          overlay.style.top = `${top}px`;
+          overlay.style.width = `${width}px`;
+          overlay.style.height = `${height}px`;
+          overlay.setAttribute('id', 'happo-screenshot-overlay');


This should probably be some sort of shared constant.

lencioni · 2017-01-04T18:08:55Z

src/server/addGeckoDriverToPath.js

+
+process.env.PATH += path.delimiter + geckodriverFolder;
+
+module.exports = geckodriverFolder;


This export doesn't seem to be used. Remove it?

Thanks! 39dd22a

lencioni · 2017-01-04T18:12:33Z

src/server/closeDriver.js

+module.exports = function closeDriver(driver) {
+  return new Promise((resolve, reject) => {
+    driver.close();
+    psNode.lookup({


Is there a way to get the pid from the driver object instead of doing this lookup?

Not that I can see. This is what I see it having:

Driver { flow_: ControlFlow { propagateUnhandledRejections_: true, activeQueue_: null, taskQueues_: Set {}, shutdownTask_: null, hold_: null }, session_: ManagedPromise { flow_: ControlFlow { propagateUnhandledRejections_: true, activeQueue_: null, taskQueues_: Set {}, shutdownTask_: null, hold_: null }, stack_: null, parent_: null, callbacks_: null, state_: 'fulfilled', handled_: true, value_: Session { id_: 'ac1a9200-3bd4-ca42-a2af-b711fe062e05', caps_: [Object] }, queue_: TaskQueue { name_: 'TaskQueue::23', flow_: [Object], tasks_: [], interrupts_: null, pending_: null, subQ_: null, state_: 'finished', unhandledRejections_: Set {} } }, executor_: Executor { w3c: true, customCommands_: Map { 'getContext' => [Object], 'setContext' => [Object] }, log_: Logger { name_: 'webdriver.http.Executor', level_: null, parent_: [Object], handlers_: null } }, fileDetector_: null, onQuit_: [Function: onQuit] }

The fact that we use an overlay div is sort of a hack, and needed to be explained better. Also used a shared constant for the element id, to prevent drift.

I added this to better debug, but forgot to remove it before pushing the commit (c9cd237).

I forgot about us only transpiling files in the server folder. To make Constants.js available to transpiled files, it needs to be in the same folder.

We were still on 47.0.1, from when we were still using the old firefox driver.

If people are using e.g. "Mpzilla Firefox 47.0.1-esr", they would see a slightly confusing error message. Including the full string is safer.

I believe this is causing build failures atm for node <= 5.

This reverts commit 270d66c. It didn't fix things. I'll try adding https://babeljs.io/docs/plugins/transform-object-rest-spread/ instead.

I'm hoping this will fix the Travis build for Node <= 5.

We were seeing this error when running tests in Node <= 5: SyntaxError: Unexpected token ... This error was coming from trying to parse this file node_modules/selenium-webdriver/lib/http.js function toExecuteAtomCommand(command, atom, ...params) { According to the readme, node 4 and 5 are supported, which makes me a little confused about having to transform them using babel. https://www.npmjs.com/package/selenium-webdriver#node-support-policy Tests seem to run a little slower after this change, but only on the first run.

This reverts commit 510262a.

lencioni · 2017-01-05T16:54:19Z

Maybe try updating to Jest 18?

trotzig · 2017-01-05T16:59:30Z

Good call. I'll try that next.

…

On Thu, 5 Jan 2017 at 17:54, Joe Lencioni ***@***.***> wrote: Maybe try updating to Jest 18? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#177 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjS5cXb9LmrmwmlCwEylHiYY8O-1D4uks5rPSA8gaJpZM4LaqhS> .

I'm hoping this will resolve the test failures we're seeing in node 4+5 at the moment.

With selenium-webdriver now only supporting 6.9.1 (LTS) and v7 [1], we can't continue supporting Node 4 and 5. I made a (270d66c) few (59c6e3b) attempts (510262a) at getting things working, but none proved to help us out. [1]: SeleniumHQ/selenium#3313

This will tell nodenv to use 6.9.1 locally. I thought about using v7 but then it would make things harder to test locally for me, since other projects use 6.9.1 and I usually have the tool `npm link`ed.

Now that we have dropped support for Node v4, we can update the pngjs package to v3.

This will make it easier to run a watch process locally: npm run --silent babel -- --watch

From what I found through local testing, these aren't needed. If you specify --silent on the original command, things will remain silent throughout.

Even though slightly more verbose, it's just as easy running npm run jest -- --watch Instead of npm run jest:watch

The `npm run` prefix isn't needed, at least based on my local testing.

Even though we bundle geckodriver, it will only work for Mac OS and Linux x64. I don't know how big of an issue this is, so I'm not worrying too much about documenting the fact that you might have to install geckodriver separately. I'll wait until that becomes an actual problem. In the meantime, we can at least mention it and link to the repo.

This reverts commit 019abe8. My assumptions were incorrect: https://travis-ci.org/Galooshi/happo/jobs/190174539

I want to give this version a final try before releasing.

trotzig · 2017-01-09T09:41:10Z

I found one issue that we should fix. But I won't let that block shipping v4.

oliviertassinari · 2017-05-29T21:38:00Z

I have been trying to port the happo-target-firefox to work with a docker bases selenium solution.
Didn't succeed as you said in the description, selenium does not support it.
That's a pretty nice finding!

trotzig added 7 commits January 4, 2017 11:41

Check firefox version before starting run

e56bcab

This will help people with old firefoxes when we switch over to geckodriver. For people who use other drivers, we bypass the check (you're on your own...).

Ignore lib folder for import-js

f38fcc4

This folder contains generated content, we don't want to import it directly.

Bump version to 4.0.0-beta.0

a2a767f

Changing the cropping behavior is a breaking change, both because other drivers (e.g. chromedriver) won't work out of the box. But also because people are going to have to upgrade firefox as part of this change.

Allow custom uploader

0cecc80

This will hopefully fix #176 through allowing the default uploader (S3) to be overridden in configuration.

Bump version to 4.0.0-beta.1

d44296c

lencioni approved these changes Jan 4, 2017

View reviewed changes

trotzig added 12 commits January 4, 2017 21:46

Better explain screenshot div

90d68b5

The fact that we use an overlay div is sort of a hack, and needed to be explained better. Also used a shared constant for the element id, to prevent drift.

Remove export from addGeckoDriverToPath.js

39dd22a

I added this to better debug, but forgot to remove it before pushing the commit (c9cd237).

Move Constants.js to server folder

17d54bd

I forgot about us only transpiling files in the server folder. To make Constants.js available to transpiled files, it needs to be in the same folder.

Remove unused import in cli.js

17d91e7

Use Firefox 50 in Travis

224a8e7

We were still on 47.0.1, from when we were still using the old firefox driver.

Print full firefox version string when too old

e16e311

If people are using e.g. "Mpzilla Firefox 47.0.1-esr", they would see a slightly confusing error message. Including the full string is safer.

Try different Firefox version for Travis

20eb9f5

Stop using object-rest-spread

270d66c

I believe this is causing build failures atm for node <= 5.

Revert "Stop using object-rest-spread"

6488f1b

This reverts commit 270d66c. It didn't fix things. I'll try adding https://babeljs.io/docs/plugins/transform-object-rest-spread/ instead.

Add babel-transform-object-rest-spread

59c6e3b

I'm hoping this will fix the Travis build for Node <= 5.

Revert "Don't exlude node-modules from being transformed for Jest"

87211c8

This reverts commit 510262a.

trotzig added 6 commits January 7, 2017 20:11

Update to jest 18

69205b9

I'm hoping this will resolve the test failures we're seeing in node 4+5 at the moment.

Add .node-version file with 6.9.1

75fbe34

This will tell nodenv to use 6.9.1 locally. I thought about using v7 but then it would make things harder to test locally for me, since other projects use 6.9.1 and I usually have the tool `npm link`ed.

Update pngjs to v3

46ee6f0

Now that we have dropped support for Node v4, we can update the pngjs package to v3.

Add npm run babel script

a31f2f4

This will make it easier to run a watch process locally: npm run --silent babel -- --watch

Remove inner --silent from npm scripts

c474999

From what I found through local testing, these aren't needed. If you specify --silent on the original command, things will remain silent throughout.

trotzig added 5 commits January 9, 2017 09:06

Remove jest:watch script

96a0245

Even though slightly more verbose, it's just as easy running npm run jest -- --watch Instead of npm run jest:watch

Remove npm run from scripts

019abe8

The `npm run` prefix isn't needed, at least based on my local testing.

Revert "Remove npm run from scripts"

21178c9

This reverts commit 019abe8. My assumptions were incorrect: https://travis-ci.org/Galooshi/happo/jobs/190174539

Bump version to 4.0.0-beta.2

188c097

I want to give this version a final try before releasing.

trotzig merged commit 1b4d49b into master Jan 9, 2017

lencioni deleted the pre-cropped-screenshots branch May 29, 2017 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre cropped screenshots #177

Pre cropped screenshots #177

trotzig commented Jan 4, 2017

trotzig commented Jan 4, 2017

trotzig commented Jan 4, 2017

lencioni Jan 4, 2017

trotzig Jan 4, 2017

lencioni Jan 4, 2017

trotzig Jan 4, 2017

lencioni Jan 4, 2017

trotzig Jan 4, 2017

lencioni Jan 4, 2017

lencioni Jan 4, 2017

trotzig Jan 4, 2017

lencioni Jan 4, 2017

trotzig Jan 4, 2017

lencioni Jan 4, 2017

lencioni Jan 4, 2017

trotzig Jan 4, 2017

lencioni Jan 4, 2017

trotzig Jan 4, 2017

lencioni commented Jan 5, 2017

trotzig commented Jan 5, 2017 via email

trotzig commented Jan 9, 2017

oliviertassinari commented May 29, 2017


		process.env.PATH += path.delimiter + geckodriverFolder;

		module.exports = geckodriverFolder;

Pre cropped screenshots #177

Pre cropped screenshots #177

Conversation

trotzig commented Jan 4, 2017

trotzig commented Jan 4, 2017

trotzig commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Jan 5, 2017

trotzig commented Jan 5, 2017 via email

trotzig commented Jan 9, 2017

oliviertassinari commented May 29, 2017