Add filename to result #78

cuff-links · 2018-04-12T22:22:14Z

This PR adds the data for the filenames of the images that were created to the returned results. This is helpful when debugging test failures so you can output whatever exact image failed. If you have 5 different viewports, it can be helpful for displaying an image or pushing the image to some kind of cloud solution.

…t in tolerance.

…se it.

cuff-links · 2018-04-16T19:37:33Z

@zinserjan Added the ability to return file data from the createReport call. Modified tests to account for this. All is green. Let me know what you think.

emilyrohrbough · 2018-04-17T18:40:30Z

package.json

@@ -40,6 +40,7 @@
    "lodash": "^4.13.1",
    "node-resemble-js": "0.0.5",
    "nodeclient-spectre": "^1.0.3",
+    "phantomjs-prebuilt": "^2.1.16",


Why was this move to a dependency?

Hmm. I think this happened when I used yarn to add it. I will switch.

cuff-links · 2018-04-18T17:14:57Z

Thanks for catching that, @emilyrohrbough.

zinserjan

Thanks for your work. I like the idea, but I think this needs to be done in a different way to make this more generic. See my comments.

zinserjan · 2018-04-19T17:51:57Z

src/methods/BaseCompare.js

@@ -126,8 +126,9 @@ export default class BaseCompare {
    return path.join(runtimeConfigPath, `${name}-${suffix}.json`);
  }

-  createResultReport(misMatchPercentage, isWithinMisMatchTolerance, isSameDimensions) {
+  createResultReport(filePaths, misMatchPercentage, isWithinMisMatchTolerance, isSameDimensions) {


Chaning the order of arguments is a breaking change for all guys who uses custom compare methods. I would like to avoid this. Moreover this is to inflexibel and only useful for file comparison. I suggest to replace filePaths with an object that wil be assigned via Object.assign to the returned object. Then custom or other compare methods can enhance the report with additional information.

@zinserjan Would you rather I made it the last argument, then? That way, if I am remembering my JS correctly, the value would just be null for those who aren't using the functionality?

zinserjan · 2018-04-19T17:52:43Z

src/methods/LocalCompare.js

@@ -25,6 +25,13 @@ export default class LocalCompare extends BaseCompare {

    const referenceExists = await fs.exists(referencePath);

+    var fileNames;


use const and assign value directly

zinserjan · 2018-04-19T17:53:30Z

src/methods/LocalCompare.js

+
+    fileNames = {
+      reference: referencePath,
+      actual: screenshotPath


add diff property with value of null to make sure the property is always there

replace key actual with screenshot to be consistent with the referenceName , screenshotName and diffName option.

…om actual to screenshots for current taken screenshot. Added diff: null to response object.

cuff-links · 2018-04-23T19:18:39Z

@zinserjan I looked at the travis build and the failure seems a little funky. Passed on node 6 failed on 8. And it didn't have anything to do with test logic failing. Unfortunately, I can't rerun the tests to see if this was just a fluke failure.

cuff-links · 2018-04-30T16:56:58Z

@zinserjan @emilyrohrbough Can someone rerun this?

cuff-links · 2018-05-03T22:02:45Z

Is this PR still missing something? I believe all points in the review were addressed. @zinserjan @emilyrohrbough

emilyrohrbough · 2018-05-03T22:08:36Z

@silne30 I don't have privs to rerun tests.

cuff-links · 2018-05-03T22:32:39Z

Gotcha. Thanks.

…

On Thu, May 3, 2018, 6:08 PM Emily Rohrbough ***@***.***> wrote: @silne30 <https://github.com/silne30> I don't have privs to rerun tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#78 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABXaDpUYtdhHDDo5O5qJ_ImILdDK76v3ks5tu3_kgaJpZM4TSlOs> .

zinserjan · 2018-05-08T15:51:11Z

@silne30 just restarted the test.
I'll review this PR later ;)

zinserjan · 2018-05-10T07:27:04Z

src/methods/Spectre.js

+      diff: null
+    };
+
+    return this.createResultReport(result.diff, result.pass, true, filenames);


Remove filesnames from spectre report. This method doesn't use any files.

zinserjan · 2018-05-10T07:39:09Z

src/methods/BaseCompare.js

@@ -126,12 +126,13 @@ export default class BaseCompare {
    return path.join(runtimeConfigPath, `${name}-${suffix}.json`);
  }

-  createResultReport(misMatchPercentage, isWithinMisMatchTolerance, isSameDimensions) {
+  createResultReport(misMatchPercentage, isWithinMisMatchTolerance, isSameDimensions, filePaths) {


Moreover this is to inflexibel and only useful for file comparison. I suggest to replace filePaths with an object that wil be assigned via Object.assign to the returned object. Then custom or other compare methods can enhance the report with additional information.

This method should be more generic like I already said. What I mean is the following:

createResultReport(misMatchPercentage, isWithinMisMatchTolerance, isSameDimensions, options = {}) { return { ...options, misMatchPercentage, isWithinMisMatchTolerance, isSameDimensions, isExactSameImage: misMatchPercentage === 0, }; }

This allows you to pass everything you want in and just merges this into the report object.

In your case you would call it the following for LocalCompare & SaveScreenshot. All other methods shouldn't be touched.

this.createResultReport(misMatchPercentage, true, isSameDimensions, { filenames: { reference: referencePath, screenshot: screenshotPath, diff: null, }, });

OK. Thanks for that. Had to look into the spread operator in ES6 because I thought ...options was a typo. XD

cuff-links · 2018-05-16T23:47:06Z

@zinserjan I think this is more what you were looking for. ^_^

cuff-links · 2018-05-24T19:49:07Z

@zinserjan Is there anything else this needs?

cuff-links · 2018-05-29T18:42:44Z

@emilyrohrbough @zinserjan I know you all are busy. Just making sure that there is nothing else that I can do on my end for this PR.

emilyrohrbough · 2018-05-29T19:01:17Z

src/methods/LocalCompare.js

@@ -25,6 +25,15 @@ export default class LocalCompare extends BaseCompare {

    const referenceExists = await fs.exists(referencePath);

+


nit: extra line

emilyrohrbough · 2018-05-29T19:01:21Z

src/methods/LocalCompare.js

@@ -38,23 +47,25 @@ export default class LocalCompare extends BaseCompare {

      const diffPath = this.getDiffFile(context);

+


nit: extra line

emilyrohrbough · 2018-05-29T19:06:22Z

src/methods/LocalCompare.js


-        return this.createResultReport(misMatchPercentage, false, isSameDimensions);
+        return this.createResultReport(misMatchPercentage, false, isSameDimensions, options);


nit: the var name options is misleading here as these are report results not config options.

emilyrohrbough · 2018-05-29T19:07:10Z

This looks good to me. Can you explain the reason for the phantom dependency change?

cuff-links · 2018-05-29T19:13:14Z

@emilyrohrbough
phantomjs was deprecated in favor of phantomjs-prebuilt.

yuderekyu · 2018-05-29T19:17:35Z

src/methods/BaseCompare.js

    return {
      misMatchPercentage,
      isWithinMisMatchTolerance,
      isSameDimensions,
-      isExactSameImage: misMatchPercentage === 0
+      isExactSameImage: misMatchPercentage === 0,
+      ...options


Nit: to be safe, wouldn't we want to spread options first? In the future, we could potentially override the other object properties if we add duplicate properties to the passed in fileNames object.

@yuderekyu Sorry. I am a bit new to ES6 and all of it's goodness. Can you explain to me what you mean by "spread options first"? First where?

return { ...options, //spread this first misMatchPercentage, isWithinMisMatchTolerance, isSameDimensions, isExactSameImage: misMatchPercentage === 0, };

Just to be safe. If options somehow contains any other keys (misMatchPercentage, isWithinMismatchTolerance, isSameDimensions, isExactSameImage), then spreading options last would override those values.

cuff-links · 2018-05-31T20:00:57Z

@emilyrohrbough @yuderekyu @zinserjan We good now?

cuff-links · 2018-06-20T19:07:20Z

Ummmm.....can this be merged now?

cuff-links · 2018-07-02T18:55:33Z

??

cuff-links · 2018-07-05T19:41:48Z

@zinserjan I have many thumbs up here. Can this change be merged yet

mgibas · 2018-07-06T06:07:43Z

Thumbocracy 🤣
For real - maintainers either merge or reject - please please please :)

cuff-links · 2018-07-09T21:44:09Z

@zinserjan @emilyrohrbough @yuderekyu Is there anything I may have missed?

emilyrohrbough · 2018-07-11T19:09:57Z

@silne30 I do not have the permissions to merge this PR. I use the wdio-visual-regression-service within a project which is why I have been active on this PR.

cuff-links · 2018-08-22T19:03:03Z

@zinserjan Hitting the 90 day mark since last fix was made. Anything else this needs?

cuff-links · 2018-09-05T15:52:59Z

Hope everything is okay. Please approve if this works. Please give me a feedback if I need to change something.

cuff-links · 2018-11-05T17:34:56Z

@zinserjan Anyone?

cuff-links · 2019-01-09T18:18:25Z

Not sure if this project has nodded off. We are looking at almost 9 months since feedback from last commit. I am not even using this tool anymore but I am hoping the PR get's pulled for anyone else who may want the functionality. @zinserjan Are you still looking into this?

cuff-links · 2019-07-06T03:32:27Z

Gonna go ahead and close this. If we need it, we can reopen it. We are going on a year without any update since the last code change.

John Dorlus added 6 commits April 12, 2018 16:08

Added filenames to be returned with results.

bb56e5a

Added filepaths for images created to the returned results.

59ccb77

Added filename to results and updated tests.

d2af165

Added to test to ensure that diff included in filepath when result no…

dfc4bab

…t in tolerance.

Updated phantomjs to phantomjs-prebuilt per npm and updated test to u…

0a9e560

…se it.

Removed hard coded directories from tests.

c39a965

emilyrohrbough reviewed Apr 17, 2018

View reviewed changes

moved phantomjs-prebuilt to devDependency as it should have been.

9150006

zinserjan requested changes Apr 19, 2018

View reviewed changes

Restored order of args for createReport(). Fixed naming convention fr…

2ac00b6

…om actual to screenshots for current taken screenshot. Added diff: null to response object.

cuff-links mentioned this pull request May 3, 2018

Integrate visual regression testing elastic/eui#767

Closed

3 tasks

zinserjan requested changes May 10, 2018

View reviewed changes

zinserjan mentioned this pull request May 10, 2018

Ability to fail test when there is no reference image #79

Open

Added object for options rather than having filepath everywhere.

a4fa2d3

ol0lll mentioned this pull request May 24, 2018

Add screenshot filename BurdaMagazinOrg/sharpeye#13

Closed

emilyrohrbough reviewed May 29, 2018

View reviewed changes

Fixed extra line nits.

e11ada3

emilyrohrbough reviewed May 29, 2018

View reviewed changes

yuderekyu reviewed May 29, 2018

View reviewed changes

Spread report options args first to prevent overwriting.

992cca3

cuff-links closed this Jul 6, 2019

		@@ -25,6 +25,13 @@ export default class LocalCompare extends BaseCompare {

		const referenceExists = await fs.exists(referencePath);

		var fileNames;

		@@ -25,6 +25,15 @@ export default class LocalCompare extends BaseCompare {

		const referenceExists = await fs.exists(referencePath);

		@@ -38,23 +47,25 @@ export default class LocalCompare extends BaseCompare {

		const diffPath = this.getDiffFile(context);


		return this.createResultReport(misMatchPercentage, false, isSameDimensions);
		return this.createResultReport(misMatchPercentage, false, isSameDimensions, options);

Add filename to result #78

Add filename to result #78

Conversation

cuff-links commented Apr 12, 2018

cuff-links commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuff-links commented Apr 18, 2018

zinserjan left a 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

cuff-links commented Apr 23, 2018

cuff-links commented Apr 30, 2018

cuff-links commented May 3, 2018

emilyrohrbough commented May 3, 2018

cuff-links commented May 3, 2018 via email

zinserjan commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuff-links commented May 16, 2018 • edited Loading

cuff-links commented May 24, 2018

cuff-links commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough commented May 29, 2018

cuff-links commented May 29, 2018 • edited Loading

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

cuff-links commented May 31, 2018

cuff-links commented Jun 20, 2018

cuff-links commented Jul 2, 2018

cuff-links commented Jul 5, 2018 • edited Loading

mgibas commented Jul 6, 2018

cuff-links commented Jul 9, 2018

emilyrohrbough commented Jul 11, 2018

cuff-links commented Aug 22, 2018

cuff-links commented Sep 5, 2018

cuff-links commented Nov 5, 2018

cuff-links commented Jan 9, 2019

cuff-links commented Jul 6, 2019

cuff-links commented May 16, 2018 •

edited

Loading

cuff-links commented May 29, 2018 •

edited

Loading

cuff-links commented Jul 5, 2018 •

edited

Loading