Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add custom jest reporter. Refs #10 #59

Closed
wants to merge 25 commits into from

Conversation

badsyntax
Copy link
Collaborator

@badsyntax badsyntax commented Nov 22, 2017

Original PR: #32

This change introduces a custom jest reporter that generates a report of the differencify tests once all tests have completed.

Requirements:

  1. Indexes all tests, saves to custom location in HTML format
  2. Includes snapshot and diff images from the test directories
  3. Includes test statuses
  4. Is unit tested
  5. Includes debug/log messages (eg, mismatch info)
  6. Includes "after" image (not currently saved to disk)

To be fixed:

  1. Current implementation does not find the diff image for whatever reason

Issues:

  1. Custom reporter config viapackage.json is not supported by CRA
  2. Only support jest tests

@NimaSoroush
Copy link
Owner

@badsyntax : should the report call happen in separate call. something like:

...
  afterAll(async () => {
    await differencify.report();
  });
...

@badsyntax
Copy link
Collaborator Author

badsyntax commented Nov 22, 2017

@NimaSoroush You don't need to explicitly call it like that, you just enable the reporter via the jest reporters config. (Check this config). The reporter is then run when all tests are complete.

The reporter will then check every test for the existence of the snapshot image and the diff image and add those tests to the report. It's probably best to have a custom test:visual npm script that only runs the reporter on the visual tests. In that case we can configure jest to use a custom reporter via the cli options (not documented on the website).

@NimaSoroush
Copy link
Owner

@badsyntax : Cool, that makes sense. I was more thinking of programmatically activation/deactivation of the report. But this looks good as the first version

@badsyntax
Copy link
Collaborator Author

@NimaSoroush With this new approach we can support multiple test files. If we have to programmatically activate the report then I don't think we can support a report that indexes multiple tests.

@NimaSoroush
Copy link
Owner

@badsyntax : even with the thing I suggested to write the report somewhere (e.g. in .txt/json file or keep it in memory like differencify.report) while executing the test, and then when executing differencify.report() it will pick up that file/variable and generate the report?

// differencify
import { reporter } from 'reporter';

class Differencify {
  async report() {
    reporter.report();
  }
  ...
}

// page.js
import { reportObj } from 'reporter';
class Page {
  ...
  async goto(url) {
  ...
  if (this.globalConfig.generateReport) {
    reportObj['debug'] = `
      ${reportObj['debug']}
      Opening new tab...`;
  }
  ...
  }
}

// compareImage.js
  import { reportObj } from 'reporter';
  compareImage = async (capturedImage, globalConfig, testConfig) => {
    ...
    reportObj = {
      summary: 'passed',
      referenceImg: './path/reference.png',
      diffImg: './path/diff.png',
    }
    

what do you think?

@badsyntax
Copy link
Collaborator Author

So I actually prefer for it to be explicit like that, and this is similar to my original approach, but I found it doesn't work well with multiple test files. I changed the approach to use a custom jest reporter to support generating a report that indexes all tests.

Let's consider the following scenario:

test1.test.js - running 
test2.test.js - running 

test2.test.js - complete - differencify.report()
test1.test.js - complete - differencify.report()

The problem with this approach is that:

  • The report is generated multiple times.
  • test1 doesn't know anything about test2 so for each differencify.report() we'd need to do a directory scan to find snapshots, diff and metadata of all other tests.
  • potential race condition between test2 and test1 trying to save the report (tests are run in parallel)

With the new approach, using a custom jest reporter, it works like so:

test1.test.js - running 
test2.test.js - running 

test2.test.js - complete
test1.test.js - complete

all tests complete - generate report

In this case, we know which exact tests were executed, so we don't need to a directory scan, and the report is only generated once.

@badsyntax
Copy link
Collaborator Author

@NimaSoroush Hit me up on Gitter or slack if you'd like to chat in more detail about this

@NimaSoroush
Copy link
Owner

@badsyntax : Hey Richard, have you got any update on this PR? I am happy to help if anything left

@@ -50,6 +50,7 @@
"check-types": "^7.2.0",
"fs": "0.0.1-security",
"jimp": "^0.2.28",
"pkg-dir": "2.0.0",

Choose a reason for hiding this comment

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

differencify version update not needed ?

Copy link
Owner

Choose a reason for hiding this comment

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

It does. Richard probably going to update this at some point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not entirely what you mean @nimishseraphim, but this branch is now up to date with master.

Copy link
Owner

Choose a reason for hiding this comment

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

He means Differencify@1.3.0 -> Differencify@1.4.0

Can you add this @badsyntax ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated this 👍

@badsyntax
Copy link
Collaborator Author

@NimaSoroush I'll update and finish of these changes this afternoon. Very sorry for the delay.

@NimaSoroush
Copy link
Owner

@badsyntax : No worries Richard. Take your time and thanks for doing this. I can also help out if you want

@badsyntax badsyntax force-pushed the custom-reporter branch 4 times, most recently from 5f19f05 to e613aa2 Compare February 28, 2018 09:09
@badsyntax
Copy link
Collaborator Author

@NimaSoroush Hi Nima. This MR is ready for review again.

reporters: [
'default',
[
'../dist/reporter',
Copy link
Owner

Choose a reason for hiding this comment

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

This is fine to be ../dist/reporter but it will break the circleCi pipeline as we don't do npm run build before npm run test:integration

Copy link
Owner

Choose a reason for hiding this comment

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

Can you update that?

@NimaSoroush
Copy link
Owner

@badsyntax : Hi Richard, Sorry to get back on this with a long delay. I just got some time to check this and put some comments.

I ran this locally and noticed that it only generates reports for failed tests? I'll add my generated files here. 2 tests passed and the rest failed, but in the report, I only see failed ones

2018-05-03_2336

Archive.zip


You can generate an index document of the saved images by using the differencify jest reporter.

Enable the reporter in your jest config:
Copy link
Owner

Choose a reason for hiding this comment

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

Can we add a link to differencify reporter example here?

NimaSoroush and others added 20 commits June 9, 2018 12:32
* Add dockerignore file to reduce build context size

* Add script to run integration tests with volume map

* Simplify npm scripts with docker one-liner

* Maintian an NPM script for running integration tests locally
…back (NimaSoroush#83)

* [NimaSoroush#82] Add support for accessing detailed test values via toMatchSnapshot

Updates `toMatchSnapshot` to accept a second argument, a callback. Upon
evaluation of the test, the test result is set on the class instance. If
`toMatchSnapshot` is given a callback, the callback is invoked upon
completion of the test and passed the full result data.

- Include additional test result data in return for verbosity
- Store result on instance and invoke callback with data if present
- Add verbosity to error and success return values

* [NimaSoroush#82] Update tests and documentation to reflect toMatchSnapshot changes

* [NimaSoroush#82] Add description of arguments for toMatchSnapshot
Fixes spelling error
Fixing issue with freeze image
* Adding awesome badge

* update
* chore(package): update dependencies

* docs(readme): add Greenkeeper badge
@NimaSoroush
Copy link
Owner

This PR can be closed in favor of #103

@NimaSoroush NimaSoroush closed this Oct 1, 2018
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.

None yet

5 participants