Skip to content

Add screenshot-comparison utility, to protect against visual regressions during CSS refactors.#7395

Merged
cjcenizal merged 1 commit intoelastic:masterfrom
cjcenizal:feature/compare-screenshots-utility
Jun 8, 2016
Merged

Add screenshot-comparison utility, to protect against visual regressions during CSS refactors.#7395
cjcenizal merged 1 commit intoelastic:masterfrom
cjcenizal:feature/compare-screenshots-utility

Conversation

@cjcenizal
Copy link
Copy Markdown
Contributor

Changes

  • Remove test/output and added test/screenshots (requires a Jenkins change).
  • Add test/screenshots/baseline images. These document the expected state of the UI.
  • Add dependency on image-diff package.
  • Add utilities/compareScreenshots.js, which can be run via 'npm run compareScreenshots'.

Reference

Based upon #7382

.gitignore Outdated
Copy link
Copy Markdown
Contributor

@spalger spalger Jun 8, 2016

Choose a reason for hiding this comment

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

I suggest using mkdirp.sync(dirname(path)) before saving. It's more future proof and less messy that way.

@spalger
Copy link
Copy Markdown
Contributor

spalger commented Jun 8, 2016

Looking good

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is a CLI tool I would just throw comparisonError.

@cjcenizal cjcenizal force-pushed the feature/compare-screenshots-utility branch from 07aa5a3 to 8f8b82e Compare June 8, 2016 19:32
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one more self to get rid of, sorry

@spalger
Copy link
Copy Markdown
Contributor

spalger commented Jun 8, 2016

LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probable move catch clause one level up, so we saveScreenshot.catch(reason => console.log(reason)). Leave error handling to consumer

@bevacqua
Copy link
Copy Markdown
Contributor

bevacqua commented Jun 8, 2016

Few style comments but LGTM otherwise

@cjcenizal cjcenizal force-pushed the feature/compare-screenshots-utility branch 3 times, most recently from 193dde3 to eb5618d Compare June 8, 2016 22:25
…ons during CSS refactors.

- Remove test/output and added test/screenshots (requires a Jenkins change).
- Add test/screenshots/baseline images. These document the expected state of the UI.
- Add dependency on image-diff package.
- Add utilities/compareScreenshots.js, which can be run via 'npm run compareScreenshots'.
@cjcenizal cjcenizal force-pushed the feature/compare-screenshots-utility branch from eb5618d to 9fa2e82 Compare June 8, 2016 22:52
@cjcenizal
Copy link
Copy Markdown
Contributor Author

@spalger Looks like Jenkins is pushing the S3 artifacts! Thanks.

@cjcenizal cjcenizal merged commit 207a481 into elastic:master Jun 8, 2016
@cjcenizal cjcenizal deleted the feature/compare-screenshots-utility branch June 8, 2016 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants