Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Pre cropped screenshots #177

Merged
merged 30 commits into from
Jan 9, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
e56bcab
Check firefox version before starting run
trotzig Nov 29, 2016
c9cd237
Limit screenshot to example box
trotzig Nov 26, 2016
9ff6879
Work around marionette bug when closing geckodriver
trotzig Jan 4, 2017
f38fcc4
Ignore lib folder for import-js
trotzig Jan 4, 2017
a2a767f
Bump version to 4.0.0-beta.0
trotzig Jan 4, 2017
0cecc80
Allow custom uploader
trotzig Jan 4, 2017
d44296c
Bump version to 4.0.0-beta.1
trotzig Jan 4, 2017
90d68b5
Better explain screenshot div
trotzig Jan 4, 2017
39dd22a
Remove export from addGeckoDriverToPath.js
trotzig Jan 4, 2017
17d54bd
Move Constants.js to server folder
trotzig Jan 4, 2017
17d91e7
Remove unused import in cli.js
trotzig Jan 4, 2017
224a8e7
Use Firefox 50 in Travis
trotzig Jan 4, 2017
e16e311
Print full firefox version string when too old
trotzig Jan 4, 2017
20eb9f5
Try different Firefox version for Travis
trotzig Jan 4, 2017
270d66c
Stop using object-rest-spread
trotzig Jan 4, 2017
6488f1b
Revert "Stop using object-rest-spread"
trotzig Jan 5, 2017
59c6e3b
Add babel-transform-object-rest-spread
trotzig Jan 5, 2017
510262a
Don't exlude node-modules from being transformed for Jest
trotzig Jan 5, 2017
87211c8
Revert "Don't exlude node-modules from being transformed for Jest"
trotzig Jan 5, 2017
69205b9
Update to jest 18
trotzig Jan 7, 2017
e8648eb
Drop support for Node 4 and 5
trotzig Jan 9, 2017
75fbe34
Add .node-version file with 6.9.1
trotzig Jan 9, 2017
46ee6f0
Update pngjs to v3
trotzig Jan 9, 2017
a31f2f4
Add `npm run babel` script
trotzig Jan 9, 2017
c474999
Remove inner `--silent` from npm scripts
trotzig Jan 9, 2017
96a0245
Remove jest:watch script
trotzig Jan 9, 2017
019abe8
Remove `npm run` from scripts
trotzig Jan 9, 2017
897eb78
Mention geckodriver in README
trotzig Jan 9, 2017
21178c9
Revert "Remove `npm run` from scripts"
trotzig Jan 9, 2017
188c097
Bump version to 4.0.0-beta.2
trotzig Jan 9, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/server/__tests__/checkBrowserVersion-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
jest.mock('child_process');
const childProcess = require('child_process');

jest.mock('../config');
const config = require('../config');

const checkBrowserVersion = require('../checkBrowserVersion');

beforeEach(() => {
config.driver = 'firefox';
});

describe('when firefox is new enough', () => {
beforeEach(() => {
childProcess.exec = (_, cb) => cb(null, 'Mozilla Firefox 50.0');
});

it('resolves', () =>
checkBrowserVersion().then(() => {
expect(true).toBe(true);
}));
});

describe('when firefox is too old', () => {
beforeEach(() => {
childProcess.exec = (_, cb) => cb(null, 'Mozilla Firefox 47.0');
});

it('rejects', () =>
checkBrowserVersion().catch((error) => {
expect(error.message).toEqual(
'Happo requires Firefox version 50 or later. You are using 47.0');
}));
});

describe('when the version string is unrecognized', () => {
beforeEach(() => {
childProcess.exec = (_, cb) => cb(null, 'Godzilla Firefox 47.0');
});

it('rejects', () =>
checkBrowserVersion().catch((error) => {
expect(error.message).toEqual(
'Failed to parse Firefox version string "Godzilla Firefox 47.0"');
}));
});

describe('when using a different driver', () => {
beforeEach(() => {
config.driver = 'chrome';

// Just to make sure that we're not falsely succeeding because of normal
// handling, we also mock an old version of Firefox.
childProcess.exec = (_, cb) => cb(null, 'Mozilla Firefox 47.0');
});

it('resolves', () =>
checkBrowserVersion().then(() => {
expect(true).toBe(true);
}));
});
4 changes: 4 additions & 0 deletions src/server/__tests__/runVisualDiffs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ const initializeWebdriver = require('../initializeWebdriver');
const runVisualDiffs = require('../runVisualDiffs');
const server = require('../server');

jest.mock('../checkBrowserVersion');
const checkBrowserVersion = require('../checkBrowserVersion');

describe('runVisualDiffs', () => {
let driver;
let startedServer;
Expand All @@ -30,6 +33,7 @@ describe('runVisualDiffs', () => {

beforeEach(() => {
originalConfig = { ...config };
checkBrowserVersion.mockImplementation(() => Promise.resolve());
});

afterEach(() => {
Expand Down
37 changes: 37 additions & 0 deletions src/server/checkBrowserVersion.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
const childProcess = require('child_process');

const firefox = require('selenium-webdriver/firefox');

const config = require('./config');

const MINIMUM_FIREFOX_VERSION = 50.0;
const FIREFOX_VERSION_MATCHER = /Mozilla Firefox ([0-9.]+)/;
Copy link
Contributor

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.

Copy link
Contributor Author

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}"`));

Copy link
Contributor

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]}`));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah - of course. yagni?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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


module.exports = function checkBrowserVersion() {
if (config.driver !== 'firefox') {
// Our main browser target is Firefox. If you are using something else, you
// must know what you are doing. :)
return Promise.resolve();
}

return new Promise((resolve, reject) => {
new firefox.Binary().locate().then((pathToExecutable) => {
childProcess.exec(`${pathToExecutable} --version`, (error, stdout) => {
if (error) {
reject(new Error(`Failed to check Firefox version: ${error}`));
return;
}
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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

reject(new Error(
`Happo requires Firefox version ${MINIMUM_FIREFOX_VERSION} or later. ` +
`You are using ${match[1]}`));
} else {
resolve();
}
});
});
});
};
8 changes: 5 additions & 3 deletions src/server/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const crypto = require('crypto');
const commander = require('commander');

const S3Uploader = require('./S3Uploader');
const checkBrowserVersion = require('./checkBrowserVersion');
const constructUrl = require('./constructUrl');
const initializeWebdriver = require('./initializeWebdriver');
const runVisualDiffs = require('./runVisualDiffs');
Expand All @@ -21,8 +22,10 @@ commander.command('debug').action(() => {
});

commander.command('run').action(() => {
server.start().then(() => {
initializeWebdriver().then((driver) => {
server.start()
.then(checkBrowserVersion)
.then(initializeWebdriver)
.then((driver) => {
runVisualDiffs(driver)
.then(() => {
driver.close();
Expand All @@ -33,7 +36,6 @@ commander.command('run').action(() => {
logAndExit(error);
});
});
});
});

commander.command('review').action(() => {
Expand Down