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

Jest-like simple interface #7

Merged
merged 3 commits into from
Apr 8, 2017
Merged

Jest-like simple interface #7

merged 3 commits into from
Apr 8, 2017

Conversation

truongsinh
Copy link
Contributor

With test and document. #6

@rjz
Copy link

rjz commented Mar 31, 2017

Thanks for this, @truongsinh -- excited to see it merged up!

One possible refinement that arises in our test suite: if we're injecting parameters anyway, we may be able to save some configuration by passing in the mocha context directly.

describe("Creating snapshot from mocha context", function() {
  it("renders correctly", function() {
    const tree = renderer.create(<h1>Hello, world</h1>).toJSON()
    expect(tree).to.matchSnapshot(this); // <= pass in context
  });
});

Later, in matchSnapshot, we could check whether we're dealing with a mocha context and build path / file accordingly.

// in matchSnapshot
snapshotFileName = snapshotFileName || internalConfig.snapshotFileName;
if (snapshotFileName.test && snapshotFileName.test.ctx instanceof mocha.Context) {
  const { title, file } = snapshotFileName.test;
  // assign (string) snapshotFileName, snapshotName, etc.
}

It wouldn't work with arrow functions, of course (documented/discouraged in mocha docs at any rate), but it may simplify configuration for suites that already rely on mocha's context.

@truongsinh
Copy link
Contributor Author

Thanks for your suggestion @rjz. I would not see configuration as a hassle though, because it's supposed to be only 2 snippets per test file

chaiJestSnapshot.registerSnapshotFileName(__filename + ".snap");

and

beforeEach(function(){
    // `this.currentTest.fullTitle()` assumes you are using mocha test runner
    chaiJestSnapshot.registerSnapshotNameTemplate(this.currentTest.fullTitle());
});

rather than passing this context every time we make an assertion. Again, the scope is to mimic jest-style as close as possible.

One way to reduce configuration further is having something like

beforeEach(function(){
    // `registerMochaContext` assumes you are using mocha test runner
    chaiJestSnapshot.registerMochaContext(this);
});

Noticed that we no longer need to register file name template. This way, we can still have expect(tree).to.matchSnapshot(); without passing the context. registerSnapshotFileName would remain for those who do not want to store snapshot files in the same place with spec files.

How does this sound?

@rjz
Copy link

rjz commented Apr 1, 2017

Awesome, @truongsinh!

@truongsinh
Copy link
Contributor Author

@rjz @suchipi @JakeSidSmith new implementation based on previous discussion

Copy link
Contributor

@JakeSidSmith JakeSidSmith left a comment

Choose a reason for hiding this comment

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

Looks awesome.

@suchipi
Copy link
Owner

suchipi commented Apr 8, 2017

Hi @truongsinh, I have given this a first pass look-over. There are some code style issues and some of the documentation/messaging is not clear, but at a glance the pattern itself looks good. I will pull this branch down to my machine tomorrow and make some tweaks and verify everything works in my environment. Just giving you an update.

@suchipi suchipi merged commit 9866fc4 into suchipi:master Apr 8, 2017
@suchipi
Copy link
Owner

suchipi commented Apr 8, 2017

Hi @truongsinh, I made some changes and merged. The new interface is available in version 0.3.0.
Here is a summary of the changes I made from your original PR:

  • Fixed styling inconsistencies
  • Replaced void 0 with undefined; added babel plugin that replaces undefined with void 0 at build time
  • Moved internalConfig into buildSnapshotScope so you don't mutate global state in tests. Renamed to config
  • Scoped the snapshot name counter to filename so that snapshot names from different files don't clobber each other
  • Fixed matchSnapshot so that it can be called with the matchSnapshot(true) signature again
  • Disallowed configuring filename without snapshot name and vice versa
  • Pulled config validation out of matchSnapshot so testing is simpler
  • Pulled template name generation out of matchSnapshot so testing it is simpler
  • Renamed registerSnapshotFilename to setFilename
  • Renamed registerSnapshotNameTemplate to setTestName
  • Renamed registerMochaContext to configureUsingMochaContext
  • Changed error messages to read better and have consistent syntax with other error messages
  • Updated README to explain different ways of using it
  • Added examples to repo

Thanks for your contribution; even though I made a lot of changes on top of it, it was a great starting point.

@rjz @truongsinh @JakeSidSmith please give the new API a try and let me know if it works for you.

@suchipi suchipi mentioned this pull request Apr 8, 2017
@rjz
Copy link

rjz commented Apr 8, 2017

Thanks, @suchipi, @truongsinh, @JakeSidSmithwe'll let you know how it goes this is working beautifully for us!

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

4 participants