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

Use scoped mocha opts file #29

Merged
merged 2 commits into from May 23, 2016
Merged

Use scoped mocha opts file #29

merged 2 commits into from May 23, 2016

Conversation

rejeep
Copy link
Collaborator

@rejeep rejeep commented May 23, 2016

(Note that this branch builds on #27, but is targeted against master, so before looking at this, merge #27)

This change will use any locally scoped mocha.opts file. This happens when there are different types of tests (unit, acceptance, integration) with their own mocha.opts file.

@@ -76,6 +77,16 @@ From http://benhollis.net/blog/2015/12/20/nodejs-stack-traces-in-emacs-compilati
(setq i (+ i 1)))
dir))

(defun mocha-opts-file (mocha-file)
"Return path to mocha.opts file for MOCHA-FILE."
Copy link
Owner

Choose a reason for hiding this comment

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

This is cool. I actually didn't know mocha supported opts files. I've just been using different .dir-locals files when I need different test configs in the same project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and it will always load test/mocha.opts even if not specified.

@rejeep rejeep force-pushed the use-scoped-mocha-opts-file branch from 803edea to 2797df7 Compare May 23, 2016 13:39

;;;; mocha-generate-command

(ert-deftest mocha-test/mocha-generate-command/return-command-including-mocha-opts-option ()
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add the negative case to this one for the integration test file? that is has no --opts option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@rejeep rejeep force-pushed the use-scoped-mocha-opts-file branch 2 times, most recently from cfcb56c to 13f7765 Compare May 23, 2016 13:50
@rejeep
Copy link
Collaborator Author

rejeep commented May 23, 2016

The tests fail for me locally with "SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode". Do you know if I have to run it with a specific version of Node?

@rejeep
Copy link
Collaborator Author

rejeep commented May 23, 2016

Node 6 solved it.

@scottaj
Copy link
Owner

scottaj commented May 23, 2016

Yeah, I just put an .nvmrc with node 6 in the sample project directory.

@scottaj
Copy link
Owner

scottaj commented May 23, 2016

How do you feel about adding a mocha.opts to the sample project and adding a cuke for this?

@rejeep
Copy link
Collaborator Author

rejeep commented May 23, 2016

How do you feel about adding a mocha.opts to the sample project and adding a cuke for this?

Jupp, will do that!

I also found a big with help of your Ecukes-tests that I will fix.

@rejeep rejeep closed this May 23, 2016
@rejeep rejeep reopened this May 23, 2016
There may be many different kind of tests with their own mocha.opts
file.  With this change, if there's a local mocha.opts file, that will
be used when running the tests.
@rejeep rejeep force-pushed the use-scoped-mocha-opts-file branch from 13f7765 to 41b8101 Compare May 23, 2016 15:41
@rejeep
Copy link
Collaborator Author

rejeep commented May 23, 2016

I started writing Ecukes tests but soon realised that I don't think we should write many of those. Adding more Node-tests changes the outcome of other features because they run tests for the complete project. Instead I think we should focus on unit tests and only have a few Ecukes tests that make sure all parts run well together.

I did some updates to this branch from your feedback, mind taking another look?

@rejeep rejeep force-pushed the use-scoped-mocha-opts-file branch from e1870bb to 41b8101 Compare May 23, 2016 16:38
@scottaj
Copy link
Owner

scottaj commented May 23, 2016

This LGTM.

As fare as cukes go, I think we'll have to make the sample project a little more complex, and maybe have multiple test directories to test different cases like CoffeeScript support. But I'm fine with deferring that till we have more major features to integrate.

@scottaj scottaj merged commit f213112 into master May 23, 2016
@scottaj scottaj deleted the use-scoped-mocha-opts-file branch May 23, 2016 20:04
@rejeep
Copy link
Collaborator Author

rejeep commented May 23, 2016

Merging this then! 👍

@rejeep
Copy link
Collaborator Author

rejeep commented May 23, 2016

And I saw you just did! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants