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: Drop whitelist config for server and test folders #4673

Merged
merged 6 commits into from
Apr 20, 2016

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 12, 2016

Part of #4596.

At the moment I removed test.json whitelist for server and test folder. We need to keep whitelist for client folder until we migrate all remaining test folders. To make everything compatible I also changes the way we enabled whitelist. It's now always enabled for client folder only.

I also renamed npm script for test folder from test-tests to test-test to align it with other subfolders.

Testing

All test runs should pass:

  • npm run test-client
  • npm run test-server
  • npm run test-test

Also for single test file:

  • npm run test-client client/lib/accept/test/index.js
  • npm run test-test test/test/helpers/use-nock/test/index.js

And finally we need to make sure that on Circle CI test runner doesn't run any tests if no test files are provided. Locally it should always load all possible tests when no test files are specified in npm command.

@gziolo
Copy link
Member Author

gziolo commented Apr 12, 2016

I guess we can already take similar step for server folder, but I noticed there is one test that wasn't whitelisted which executes only when ENV is set for desktop. Which is very strange...

@gziolo gziolo mentioned this pull request Apr 12, 2016
6 tasks
@gziolo
Copy link
Member Author

gziolo commented Apr 12, 2016

Hmm, client test failed. No idea why, I'll look into that tomorrow ;)

@gziolo gziolo force-pushed the test/drop-whitelist-files branch from 8c453e8 to 6a41f78 Compare April 13, 2016 05:26
@gziolo
Copy link
Member Author

gziolo commented Apr 13, 2016

Circle CI is green again, but I have no idea why those tests failed before. I couldn't reproduce locally.

I dropped now also tests.json whitelist from server folder.

@gziolo gziolo force-pushed the test/drop-whitelist-files branch from 82469ab to 3659822 Compare April 13, 2016 08:13
@gziolo gziolo changed the title Test: Drop whitelist config for test folder Test: Drop whitelist config for server and test folders Apr 13, 2016
@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 13, 2016
@gziolo
Copy link
Member Author

gziolo commented Apr 13, 2016

@aduth @blowery: I added some tweaks which allow us to merge this PR right away :)

const sandbox = sinon.sandbox.create();
let app, config, localRequest;

before( () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to be on a safe side and require all stuff here. I opened follow up #4697 to refactor this very desktop dependent test.

@gziolo gziolo force-pushed the test/drop-whitelist-files branch from 3659822 to 3b4dd45 Compare April 13, 2016 08:27
parallel: true
files:
- server/**/test/*.js
- server/**/test/*.jsx
- NODE_ENV=test make client/config/index.js && MOCHA_FILE=./test-results-tests.xml npm run test-tests -- --reporter=mocha-junit-reporter -w:
- NODE_ENV=test make client/config/index.js && MOCHA_FILE=./test-results-test.xml npm run test-test -- --reporter=mocha-junit-reporter:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make the building of the config an earlier part of the run and stop repeating it?

I feel like I tried it and circle did some weird things.. Might be worth trying again to DRY this up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give it a try :)

@gziolo gziolo mentioned this pull request Apr 14, 2016
@gziolo gziolo force-pushed the test/drop-whitelist-files branch from 7dcefac to eb863cc Compare April 14, 2016 06:33
@gziolo
Copy link
Member Author

gziolo commented Apr 14, 2016

I moved config generation to pre block and it works properly 🎉

@blowery
Copy link
Contributor

blowery commented Apr 20, 2016

🚢

@blowery blowery 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 20, 2016
@gziolo gziolo merged commit 8b2b773 into master Apr 20, 2016
@gziolo gziolo deleted the test/drop-whitelist-files branch April 20, 2016 11:44
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.

2 participants