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

Test: Port lib/terms tests over to single runner #4053

Merged
merged 5 commits into from
Mar 16, 2016
Merged

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Mar 15, 2016

This one was pretty complicated because of module-level mock work.
When you turn on Mockery, we clean out the module cache. That means if you need to test
the interactions between an external singleton (say a Dispatcher) and the module under test,
you need to re-require both the Dispatcher and the module under test. This way, they
both are working with the same instance of the singleton.

If you don't re-require the Dispatcher, when you try to test dispatch through it, the module
under test will never see the dispatch. This is because the instance that you're using in the test
is different from the instance the module under test is listening to.

I also removed all of the usage of the fake dom in these tests because it doesn't appear they're actually using them.

This one was pretty complicated because of module-level mock work.
When you turn on Mockery, we clean out the module cache. That means if you need to test
the interactions between an external singleton (say a Dispatcher) and the module under test,
you need to re-require both the Dispatcher _and_ the module under test. This way, they
both are working with the same instance of the singleton.

If you don't re-require the Dispatcher, when you try to test dispatch through it, the module
under test will never see the dispatch. This is because the instance that you're using in the test
is different from the instance the module under test is listening to.

I also removed all of the usage of the fake dom in these tests because it doesn't appear they're actually using them.
@gziolo gziolo added this to the Framework: Single test runner milestone Mar 15, 2016
@blowery blowery changed the title Test: Post lib/terms tests over to single runner Test: Port lib/terms tests over to single runner Mar 15, 2016
@blowery blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 15, 2016
@blowery blowery self-assigned this Mar 15, 2016
@@ -30,6 +31,7 @@ if ( program.grep ) {
mocha.suite.beforeAll( function() {
Chai.use( immutableChai );
Chai.use( sinonChai );
sinon.assert.expose( Chai.assert, { prefix: '' } );
Copy link
Member

Choose a reason for hiding this comment

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

http://sinonjs.org/docs/#assert-expose - this is really nice feature! 👍

Copy link
Member

Choose a reason for hiding this comment

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

My second thought is that sinon-chai (http://sinonjs.org/docs/#assertions) in most cases duplicates what sinon.assert.expose offers (https://github.com/domenic/sinon-chai/blob/master/lib/sinon-chai.js#L117) ...

We should figure out if it makes sense to keep both approaches or recommend in docs one of them :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was mostly a stopgap to avoid having to do too much surgery on existing tests. I'd started to move over to the expect syntax and then chickened out. 🐔

@gziolo
Copy link
Member

gziolo commented Mar 16, 2016

Great job! I added 2 commits with some minor changes. If you like them you can merge this PR. I'm giving thumbs up 👍

Let me know if you like the idea of moving non-test files to subfolders. As I understand we are going to use pattern matching for test files (sth like client/**/test/*.js). In that case we should make sure we are able to filter out non-test files.

@blowery
Copy link
Contributor Author

blowery commented Mar 16, 2016

Let me know if you like the idea of moving non-test files to subfolders. As I understand we are going to use pattern matching for test files (sth like client/*/test/.js). In that case we should make sure we are able to filter out non-test files.

I think that's essential. Could you add it to the master move post I put up on the p2?

@gziolo
Copy link
Member

gziolo commented Mar 16, 2016

Sure, doing it now.

blowery added a commit that referenced this pull request Mar 16, 2016
Test: Port lib/terms tests over to single runner
@blowery blowery merged commit a925004 into master Mar 16, 2016
@blowery blowery deleted the test/lib-terms branch March 16, 2016 19:11
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants