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

Testing: Bring back Mocha integration with IDEs #6076

Closed
gziolo opened this issue Jun 16, 2016 · 13 comments
Closed

Testing: Bring back Mocha integration with IDEs #6076

gziolo opened this issue Jun 16, 2016 · 13 comments
Labels
Testing [Type] Bug When a feature is broken and / or not performing as intended

Comments

@gziolo
Copy link
Member

gziolo commented Jun 16, 2016

Part of #4915.

We had working integration with tooling such as Webstorm. It no longer works properly when you execute command like:
NODE_ENV=test NODE_PATH=client:test node_modules/.bin/mocha test/run-mocha.js

Latest code looks like:
https://github.com/Automattic/wp-calypso/blob/db8925cb20a8744c07be8c82dc7e4edfb086fbc6/test/run-mocha.js

It would be great to bring integration back and make sure documentation is up to date.

@gziolo gziolo added [Type] Bug When a feature is broken and / or not performing as intended Testing labels Jun 16, 2016
@gziolo gziolo added this to the Framework: Single test runner milestone Jun 16, 2016
@umurkontaci
Copy link
Contributor

I think the regression might be introduced in 336f74b , it's the commit where the script started to return 0 passing messages. Hope this helps ✌️

@gziolo
Copy link
Member Author

gziolo commented Jun 16, 2016

Yup, that should be it, I execute Mocha from terminal integration in PhpStorm, so I never played with this script ;)

@umurkontaci
Copy link
Contributor

Oh I see, are you able to get the parsed and categorized results displayed inside the IDE with terminal integration?

@gziolo
Copy link
Member Author

gziolo commented Jun 16, 2016

Nope, that's why I'm interested in learning how to integrate it better :D

@umurkontaci
Copy link
Contributor

Gotcha, hope to see it back soon :)

@samouri
Copy link
Contributor

samouri commented Jun 16, 2016

Do we need an extra file for IDE integration? Can we run lines like:
NODE_ENV=test NODE_PATH=test:client TEST_ROOT=client test/runner.js directly?

@gziolo
Copy link
Member Author

gziolo commented Jun 16, 2016

does it work this way?

@samouri
Copy link
Contributor

samouri commented Jun 16, 2016

Yessir :) -- this way we don't need to maintain any extra files / duplicate logic.

If you check out the scripts in package.json you can see exactly what is being run when we type npm run test: https://github.com/Automattic/wp-calypso/blob/master/package.json#L135-L138
PR to update documentation: #6087

EDIT: Does NOT work this way. I misunderstood how Webstorm needs to be configured

@samouri samouri added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 16, 2016
@samouri samouri self-assigned this Jun 16, 2016
@gziolo
Copy link
Member Author

gziolo commented Jun 16, 2016

I might be wrong, but I don't think it's enough to integrate with PhpStorm or Webstorm, the way @umurkontaci previously did it. It looks like to make it happen you need to have script executed by the node_modules/mocha/bin/_mocha test runner which. It is required to use PhpStorm2016.1/NodeJS/js/mocha-intellij/lib/mochaIntellijReporter.js with reporter. Otherwise you get errors like:

[IDE integration] Unexpected executable wp-calypso/test/runner.js: expected basename is "_mocha"
[IDE integration] Unexpected executable wp-calypso/test/runner.js: expected last directory name is "bin"
[IDE integration] Can not load base reporter from ./lib/reporters/base.js { [Error: Cannot find module 'wp-calypso/lib/reporters/base.js'] code: 'MODULE_NOT_FOUND' }

@gziolo
Copy link
Member Author

gziolo commented Jun 16, 2016

Related PR for reference: #4111.

@umurkontaci
Copy link
Contributor

You are indeed right @gziolo , the entrypoint has to be the mocha executable and the first argument is the script has includes all the tests.

I think there are a couple of approaches here, if we are to remove the secondary file, we might do a check to see if the script is being imported or running standalone.

That being said, we now have a pretty structured test folder structure, we might be able to have one script that does the environment setup with mocha --require and use a pattern like .*\/test\/.+.jsx? as the test parameter and mocha can do it's own discovery.

The second approach still requires maintaining another file or a piece of code, but it'll also let us run tests selectively within integrations.

@samouri samouri removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 17, 2016
@samouri
Copy link
Contributor

samouri commented Jun 17, 2016

Thanks @umurkontaci / @gziolo, you two were very right. I was able to get IDE integration by following the first approach (performing the referenced check). While it forces us to 'require' every test file, we can pass in configurations to only run specific test-suites. Here is an example of only running PostEditor tests:

screen shot 2016-06-17 at 11 44 43 am
screen shot 2016-06-17 at 11 44 17 am

Code diff

@gziolo
Copy link
Member Author

gziolo commented Sep 29, 2017

It's no longer an issue since we migrated to Jest. See #16943.

@gziolo gziolo closed this as completed Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

No branches or pull requests

3 participants