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.

Sign up for GitHub

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

Jump to bottom

Test: Move lib/reader* to single runner #3973

Merged
merged 5 commits into from
Mar 11, 2016
Merged

Test: Move lib/reader* to single runner #3973

merged 5 commits into from
Mar 11, 2016

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Mar 11, 2016

Also remove chai-immutable and add our own immutable-chai helper. chai-immutable is busted and overrides equal, which is just a bad idea.

@blowery blowery added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 11, 2016
@blowery blowery added this to the Framework: Single test runner milestone Mar 11, 2016
@blowery blowery self-assigned this Mar 11, 2016
@blowery
Copy link
Contributor Author

blowery commented Mar 11, 2016

To test, this should just pass tests. No real functional changes.

@blowery blowery force-pushed the update/move-reader-stores-to-single-runner branch from e374eef to 4efa0ea Compare March 11, 2016 01:34
@gziolo gziolo mentioned this pull request Mar 11, 2016
Closed
@blowery
Test: Move lib/reader* to single runner
8495ac1 
Also remove chai-immutable and add our own immutable-chai helper. chai-immutable is busted and overrides equal, which is just a bad idea.
@blowery blowery force-pushed the update/move-reader-stores-to-single-runner branch from 4efa0ea to 8495ac1 Compare March 11, 2016 01:53
gziolo
gziolo reviewed Mar 11, 2016
View reviewed changes
client/tests.json
@@ -33,6 +33,30 @@
},
"test": [ "assembler", "index", "store" ]
},
"reader-comment-email-subscriptions": {
Copy link
Member

Choose a reason for hiding this comment

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

I started to rename all test files to mirror file under test and to reword suite description to have the same name as file. That way we can easily find failing tests because test output directly translates into path. Let me know what do you think about this idea. We definitely can skip that in this PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that makes sense. I was also thinking about making all of these index but ... 🐔

@gziolo
Copy link
Member

gziolo commented Mar 11, 2016

I refactored one of the tests to use useFakeDom helper in 6633318. I tested also tests in isolation one by one using .only Mocha feature. This is ready to merge. I also left some comments, but they aren't that important :)

@gziolo gziolo added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 11, 2016
@gziolo gziolo mentioned this pull request Mar 11, 2016
Merged
blowery
blowery reviewed Mar 11, 2016
View reviewed changes
client/lib/reader-teams/test/team-store-test.js

describe( 'team-store', function() {

useFakeDom();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I missed that one!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes me wonder why the TeamStore needs a DOM at all...

Copy link
Member

Choose a reason for hiding this comment

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

This is a really good question :)
I should check if we can remove it completely.

@blowery blowery force-pushed the update/move-reader-stores-to-single-runner branch from 6633318 to e5c05f5 Compare March 11, 2016 15:18
blowery added 2 commits March 11, 2016 10:25
@blowery
Rename some tests to match what they test
122195a
@blowery
Add in a new helper for sinon timers and use it.
bc4506f 
Also give the function the helpers use a helpful name. This appears when those helpers bomb during test runs.
@blowery
Copy link
Contributor Author

blowery commented Mar 11, 2016

@gziolo Could you give this another look? I wasn't happy about how the team test was using timers, so I added a new helper for sinon.useFakeTimers

@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] Ready to Merge labels Mar 11, 2016
@gziolo gziolo added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 11, 2016
@gziolo
Copy link
Member

gziolo commented Mar 11, 2016

Ooops, I commented directly commits :)
I left some feedback. It looks great, nice refactorings 👍

blowery added a commit that referenced this pull request Mar 11, 2016
@blowery
Merge pull request #3973 from Automattic/update/move-reader-stores-to…
0ccf96b 
…-single-runner

Test: Move lib/reader* to single runner
@blowery blowery merged commit 0ccf96b into master Mar 11, 2016
@blowery blowery deleted the update/move-reader-stores-to-single-runner branch March 11, 2016 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@blowery blowery

Labels
Framework
Projects
None yet
Milestone
Framework: Single test runner
Development

Successfully merging this pull request may close these issues.
3 participants
@blowery @gziolo @nylen