Replacing node-resemble-js

[brendonbribeiro]

Why remove node-resemble-js?

• It doesn't find small color differences
  • Report doesn't find color difference #850
• This repository is 'LOOKING FOR MAINTAINER' and have not been updated recently
• We can use ResembleJS directly

Breaking changes

• resembleOutputOptions was replace by resembleOptions. Read https://github.com/HuddleEng/Resemble.js#nodejs for all the options

resembleOptions : {
  output: {
    errorColor: {
      red: 255,
      green: 0,
      blue: 255
    },
    errorType: "movement",
    transparency: 0.3,
    largeImageThreshold: 1200,
    useCrossOrigin: false,
    outputDiff: true
  },
  scaleToSameSize: true,
  ignore: "antialiasing"
};

[garris]

This looks pretty good. Is this working well on your side?

I guess we should still add back jpeg support. ok to add a jpeg library if the new resemble does not already support that.

[brendonbribeiro]

Pushed changes

• Enable resembleOptions. Here's an example:

resembleOptions : {
  output: {
    errorColor: {
      red: 255,
      green: 0,
      blue: 255
    },
    errorType: "movement",
    transparency: 0.3,
    largeImageThreshold: 1200,
    useCrossOrigin: false,
    outputDiff: true
  },
  scaleToSameSize: true,
  ignore: "antialiasing"
};

• Default options:

resembleOptions : {
  output: {
    transparency: 0.3,
    largeImageThreshold: 0
  },
  ignore: "nothing"
};

To Do

• Update DOCS
• Update Tests
• Remove node-resemble-js from dependencies

Questions

• What's the purpose of compare-hash.js? I made some tests on master, and this module always throw an exception.
• After this changes, the default ignore option will be nothing.
  • smoke-test is failing (this was expected). https://drive.google.com/file/d/16T7CSoYC7q2Of49POq3HSPirHzh-_BzM/view?usp=sharing
  • I an running on Mac, should I run this test on Docker to approve?
  • Should I increase misMatchThreshold?
• What do you mean by add back jpeg support?
  • fs.writeFileSync(failedDiffFilename, data.getBuffer()) will save the file, no matter the extension
  • Seems like all engines saves screenshots as png

@garris Sorry for all this questions.

[garris]

@brendonbarreto
responses to questions

• compare-hash will throw exception if image data is not exact -- this is an optimisation that saves time if most tests are passing.

• yes. please use docker for smoke-tests (the reference files that currently exist do not use docker so they will fail. This means you will be creating the first docker files for the smoke test. If I run your smoke test (also with docker) I expect it to pass with a very low threshold.

• please create a simple test which uses the .jpg option so we can be sure that this feature is still working.

Great work -- cheers!

[brendonbribeiro]

• I wrote a simple test for jpg support
  • outputFormat.js
• Updated DOCS
• removed node-resemble-js dependency

smoke-test with docker

There's an issue with running --docker and --config together (#849 (comment)). Because of that, i can't run smoke-test with docker.

docker run --rm -it --mount type=bind,source="/Users/brendonbarreto/Projetos/BackstopJS/test/configs",target=/src backstopjs/backstopjs test "--_=test" "--h=false" "--help=false" "--v=false" "--version=false" "--i=false" "--config=backstop_features" "--moby=true" "--config=backstop_features" "--moby"

Since bitmaps_reference are not affected with this changes, I think that in another PR we can:

• Fix duplicated docker arguments
• Update smoke-test to use docker

@garris Is there something else that I should do in this PR?

[garris]

This is looking good. I fixed issue #849 so please rebase off of master to make sure you’re up to date. I also realized the docker option won’t work with the current smoke test because the smoke test is run off of the file system. So please just rebase and then I can pull in the changes.

[brendonbribeiro]

Rebase done. smoke-test is failing, can I approve it on Mac (since docker isn't an option)?

[garris]

If it’s failing because of minor rendering issues (because of hardware) then you are done! Just let me know if thats the case.

[brendonbribeiro]

Please have a look at this report: https://drive.google.com/file/d/16T7CSoYC7q2Of49POq3HSPirHzh-_BzM/view?usp=sharing

Most of then are small differences, but in some cases they look a lot different.

Reference	Test

At some point the images were changed, but smoke-test could not find the differences or changes were not approved. Thats my theory. But in general, most of the tests that are failing, have just small font issues

[brendonbribeiro]

@garris I fix all conflicts. The issue that @zigotica reported was a miss configuration. So when you have time, this is ready to review.

Thanks!

[garris]

Ok. Great. I will take another look soon.

[garris]

I just wanted to bump this -- I am sorry I have not taken another look -- I have been completely slammed at work. I have booked some time on Friday to catch up with BackstopJS work and will review then.

[brendonbribeiro]

No problem, thank you for letting me know.

[kbo4sho]

The changes here will result in a new release?

[aliu145]

@brendonbarreto @garris just wondering what is the status of this merge request? I am also running into issue where Backstop does not catch difference in color between 2 screenshots

[brendonbribeiro]

From my side is ready, as it has been some time that I did, there are some conflicts to solve.

[leonard84]

@garris any chance for this to get merged? We need the real ignoreNothing setting with the tolerances set to 0. In node-resemble-js they are still at 16.

[Vince-F]

Hello @garris
First of all thanks for BackstopJS it is a great tool for non-reg testing.
As some other people I run into problems where the threshold of node-resemble-js was too high and was passing on color changes that were clearly visible to the naked eye.
Do you intend to merge this pull request please?