NimaSoroush / differencify

Differencify is a library for visual regression testing
MIT License
634 stars 46 forks source link

Extend NodeEnvironment to avoid side effect methods #62

Open NimaSoroush opened 6 years ago

NimaSoroush commented 6 years ago

Based on jest-puppeteer-example it would be great to extend NodeEnvironment similar to this

for example, this will avoid differencify.launchBrowser() and differencify.cleanup() on this example

(async () => {
  await differencify.launchBrowser();
  const target = differencify.init({ testName: 'Differencify simple unchained', chain: false });
  const page = await target.newPage();
  await page.goto('https://github.com/NimaSoroush/differencify');
  await page.setViewport({ width: 1600, height: 1200 });
  await page.waitFor(1000);
  const image = await page.screenshot();
  const result = await target.toMatchSnapshot(image);
  await page.close();
  console.log(result) // True or False
  await differencify.cleanup();
})();

@xfumihiro: Anything to add?

xfumihiro commented 6 years ago

@NimaSoroush do you want differencify to act as a Jest plugin, which is what jest-puppeteer-example is trying to do. If so, then it won't support other test frameworks.

NimaSoroush commented 6 years ago

@xfumihiro : Differencify is independent to test framework and it could be integrated with any test runner, but my goal is to optimise for Jest. As you can see I've added special functionalities for Jest like custom matcher. I can look into integrating other test runners (e.g. Mocha, Jasmine, ...) in future if any feature request came, but current AFAIK all users of this lib are Jest runners. So what I want to achieve as part of this card is to avoid launchBrowser and cleanup call for Jest users. Does that make sense?

xfumihiro commented 6 years ago

AFAIK Jest handles test suite runtimes differently than others so I had to make these PRs (Add Global Setup/Teardown options, Add Async Test Environment APIs) to make it work with global hooks. It would make sense if Differencify is split up as core & adapters.

NimaSoroush commented 6 years ago

@xfumihiro : I like the idea of splitting Differencify into core & adapters 👍 I think it should be enough for jest to expose the CustomEnvironment and user could just use it in package.json or jest.config.js. Something like

// jest.config.js
import Differencify, { JestEnvironment } from 'differencify'; 

module.exports = JestEnvironment;

or

// package.json
...
"jest": {
    "testEnvironment": "/node_modules/differencify/dist/JestEnvironment.js"
  },
...

That should be enough. right?

xfumihiro commented 6 years ago

@NimaSoroush If you want to launch/close puppeteer once upon all test suites in Jest, you should do that inside Global setup/teardown scripts (not inside test environments as I've explained here)

yq314 commented 5 years ago

Hi @NimaSoroush I'm Qing, I found out the cause of the issue I reported to you earlier over slack: when Differencify is instantiated in a sub class of NodeEnvironment, it can't access expect for some reason. So the isJest() call returns false and the tests are run in valina node mode (test names are set to test 1, test 2 etc. and jest-reporter can't insert results into the generated html).

One thing I don't understand is that even though differencity.init() is called in separate tests it still can't detect expect, to made it work I had to remove my custom NodeEnvironment and instantiate differencity in the test file.

NimaSoroush commented 5 years ago

Hi @yq314 , That is interesting! Is there anything else we can use in NodeEnvironment that would help us with detecting the Jest environment?

yq314 commented 5 years ago

@NimaSoroush when it's running in NodeEnvironment or its subclass would indicate that it's already running in jest environment? So we could always set isJest() to true ?

NimaSoroush commented 5 years ago

Yep, but we need an automated way of detecting the environment! another way to tackle this would be to introduce an aditional flag that could manually tell differencify is running on Jest

const differencify = new Differencify({ testRunner: 'jest' });
yq314 commented 5 years ago

That's a good idea indeed!

NimaSoroush commented 5 years ago

Do you mind adding a PR for that?

yq314 commented 5 years ago

I'd love too, but I want to get clarification on something first. As pointed out here https://github.com/NimaSoroush/differencify/issues/62#issuecomment-355450912 we should do launchBrowser() and cleanup() in the global startup and teardown script, however the differencify instance needs to be created somewhere else, because instance created in global startup script can't be accessed in test scripts.

possible to make differencify a global instance so I don't need to new it?

Similar to puppeteer:

const puppeteer = require('puppeteer');

It would be easier if we have:

const differencify = require('differencify');

And in this case we wouldn't need a NodeEnvironment, so the manual setting of isJest would be unnecessary too.

NimaSoroush commented 5 years ago

I have never tried that but that seems to be possible but needs a little bit of extra work. I know puppeteer provides a way of pointing to chrome instance path through browserWSEndpoint. that might be the way to achieve it. Differencify already support that through lunch options https://github.com/NimaSoroush/differencify/blob/master/src/index.js#L21 Is that something you looking for?