-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
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.
@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. |
I also need to update the README a little I believe. |
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that when we know we need to, right? For now, this should be enough I think.
const config = require('./config'); | ||
|
||
const MINIMUM_FIREFOX_VERSION = 50.0; | ||
const FIREFOX_VERSION_MATCHER = /Mozilla Firefox ([0-9.]+)/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we do, don't we?
reject(new Error(`Failed to parse Firefox version string "${stdout}"`));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I was thinking of this line:
`You are using ${match[1]}`));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - of course. yagni?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is written, people will potentially see a misleading error message, which seems like it should be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it print the full string instead: e16e311
@@ -170,6 +170,15 @@ window.happo = { | |||
width, | |||
} = getFullRect(rootNodes); | |||
|
|||
const overlay = document.createElement('div'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. 90d68b5
overlay.style.top = `${top}px`; | ||
overlay.style.width = `${width}px`; | ||
overlay.style.height = `${height}px`; | ||
overlay.setAttribute('id', 'happo-screenshot-overlay'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be some sort of shared constant.
|
||
process.env.PATH += path.delimiter + geckodriverFolder; | ||
|
||
module.exports = geckodriverFolder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This export doesn't seem to be used. Remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 39dd22a
module.exports = function closeDriver(driver) { | ||
return new Promise((resolve, reject) => { | ||
driver.close(); | ||
psNode.lookup({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to get the pid from the driver
object instead of doing this lookup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Maybe try updating to Jest 18? |
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.
I found one issue that we should fix. But I won't let that block shipping v4. |
I have been trying to port the |
No description provided.