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: Remove legacy test runner #4596

Merged
merged 12 commits into from
Apr 21, 2016
Merged

Test: Remove legacy test runner #4596

merged 12 commits into from
Apr 21, 2016

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 7, 2016 •
edited
Loading

Part of #3822.

Tasks done:

  • Remove all code related to legacy test runner and its references.
  • Improve the way npm test and make test are configured.
  • Drop tests.json from client folder and remove whitelist logic.
  • Move files distribution for Circle CI nodes to single test runner to avoid patter matching issues.
  • Remove missed Makefile files. See Framework: Migrate tests from Makefile to single test runner #3942 (comment).
  • Migrate client/components/feature-example

Testing

Run locally:

  • npm test

Check on Circle CI Test Summary:
screen shot 2016-04-21 at 10 04 11

Check on Circle CI if tests are distributed properly between nodes. You can download reports from Artifacts tab:
screen shot 2016-04-21 at 10 03 06

@gziolo gziolo self-assigned this Apr 7, 2016
@gziolo gziolo added this to the Framework: Single test runner milestone Apr 7, 2016
@gziolo gziolo force-pushed the try/remove-legacy-test-runner branch 5 times, most recently from 4cfde67 to 8e2115d Compare April 7, 2016 19:27
@gziolo gziolo mentioned this pull request Apr 7, 2016
Closed
19 tasks
@gziolo
Copy link
Member Author

gziolo commented Apr 12, 2016

@aduth @blowery: Very similar to #4673. Another cleanup task that we should have ready when all tests are migrated :)

@gziolo gziolo mentioned this pull request Apr 12, 2016
Merged
@gziolo gziolo force-pushed the try/remove-legacy-test-runner branch 6 times, most recently from be82e57 to 8ab2880 Compare April 20, 2016 16:08
@gziolo
Copy link
Member Author

gziolo commented Apr 20, 2016

@aduth @blowery I call it the day. I didn't manage to finish this one. There is one more test that needs to be migrated. We have also still issue with Circle CI pattern matching. We have two files included in client test runner which have /abtest/ in path. Any ideas how to tackle it? Should we build our own logic for parallel tests execution?

gziolo
gziolo reviewed Apr 20, 2016
View reviewed changes
test/runner.js
@@ -36,21 +35,9 @@ if ( program.grep ) {
mocha.grep( new RegExp( program.grep ) );
}

if ( process.env.CIRCLECI ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to get rid of it, but it looks like some tests are still bombing without it.

@gziolo gziolo mentioned this pull request Apr 20, 2016
Closed
@gziolo gziolo force-pushed the try/remove-legacy-test-runner branch 3 times, most recently from a32e4c3 to 02beef0 Compare April 21, 2016 07:49
@gziolo gziolo force-pushed the try/remove-legacy-test-runner branch from 52f0dab to e400145 Compare April 21, 2016 08:36
@gziolo gziolo 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 Apr 21, 2016
@gziolo
Copy link
Member Author

gziolo commented Apr 21, 2016

/cc @aduth @blowery @mtias @nb

I removed lots of code we no longer need. I hoped we need less changes to finish unit tests migration, that's why I didn't split this PR into smaller tasks. There aren't many additions so it should be good to review as one batch of changes. Let me know if I should split anything into its own PR.

nb
nb reviewed Apr 21, 2016
View reviewed changes
client/components/feature-example/test/index.js
const featureExample = shallow( <FeatureExample /> );
assert.equal( 1, featureExample.find( '.feature-example' ).length );
Copy link
Member

Choose a reason for hiding this comment

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

The expected value is usually first.

Copy link
Member Author

@gziolo gziolo Apr 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, lenghtOf is tricky:

.lengthOf(object, length, [message])

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I get it now – the grammar would be wrong if the length was the first argument.

@gziolo gziolo force-pushed the try/remove-legacy-test-runner branch from 3aa17ba to 507db47 Compare April 21, 2016 10:13
@Tug
Copy link
Contributor

Tug commented Apr 21, 2016

Nice work here. All tests passed and ran in 25s on my computer!
Code LGTM 👍

@Tug Tug 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 Apr 21, 2016
@Tug
Copy link
Contributor

Tug commented Apr 21, 2016 •
edited
Loading

Would be nice to be able to run a subset of the tests in a next PR. I tried the -g LoginTest option and it did work if I call the runner directly but not from npm test. Would be cool to watch a directory also.

@gziolo
Copy link
Member Author

gziolo commented Apr 21, 2016

@Tug Thanks for review and all feedback. Great ideas, feel free to add them to the list here 👍

@nb nb merged commit 7e7c801 into master Apr 21, 2016
@nb nb deleted the try/remove-legacy-test-runner branch April 21, 2016 11:13
@nb
Copy link
Member

nb commented Apr 21, 2016

🎉

@gziolo gziolo mentioned this pull request Apr 21, 2016
Closed
@aduth aduth mentioned this pull request Jun 13, 2016
Merged
@gwwar gwwar mentioned this pull request Oct 27, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gziolo gziolo

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

Successfully merging this pull request may close these issues.
4 participants
@gziolo @Tug @nb @scruffian