Allow local modules to work as UIs #1240

giggio · 2014-06-17T19:43:56Z

(Superseded by #1267)

The current way dependencies are handled forces us to have a ui interface (or reporter) installed globally.
I am trying to write another ui interface and unless I use an absolute path or install the module globally I can't load the ui interface. The ideal scenario is to do this per application, so when someone clones the repo the tests just work. The current way will force the developer to globally install mocha and also globally install the module I am writing with the custom ui interface.
This happens because when you require a name, the lookup starts from the executable file location, mocha (global node_modules dir). It doesn't do the lookup from the node_modules folder where the mocha command is ran (pwd).

https://github.com/visionmedia/mocha/blob/755f05410d8bdb1218073b74755089998b98a0ca/lib/mocha.js#L153

Currently mocha adds the cwd to modules.path on _mocha like so:

https://github.com/visionmedia/mocha/blob/755f05410d8bdb1218073b74755089998b98a0ca/bin/_mocha#L152

module.paths.push(cwd, join(cwd, 'node_modules'));

But not on mocha.js.

This will only work for immediate requires, only when they are required directly from _mocha. When mocha.js tries to load the new ui interface later it can't find it, because its module.paths is different from _mocha's module.paths.
To be able to load from a local node_modules folder it would be needed to set module.paths from where the require for the ui is set, at mocha.js.
This PR does just that.

Update (Jul 11): Reporters already work, see discussion bellow.
Update (Jul 12): Actually reporters don't work, discussion bellow. This PR was not merged. I messed the code up with a push --force and started again with #1267.

Fixed: - Pending test dots not being counted (leading to longer rows of dots when containing pending tests) - Off-by-one error on first dot row (first row always lacks a dot)

Make sure that a test case returning a promise fails if the promise is rejected without a reason.

giggio · 2014-06-17T19:44:08Z

I had also made some comments on #1022.

This depends on a PR made on the mocha project: mochajs/mocha#1240

jsdevel · 2014-06-19T06:55:33Z

lib/mocha.js

@@ -62,6 +64,7 @@ function image(name) {
 */

 function Mocha(options) {
+  module.paths.push(cwd, join(cwd, 'node_modules'));


@giggio would this work outside of the Mocha constructor? It seems like it has impact outside the scope of the constructor.

It would impact the whole module defined by the mocha.js file, but not any of its required modules. So, any require call made inside this file would use the extra paths. Therefore, calls to reporters and UIs would consider the added path, the cwd and node_modules.

Given that, I wonder if this belongs outside of the constructor. It would seem more intuitive to me if this were defined outside the constructor somewhere within the file.

I don't believe that the correct place would be outside the constructor, as the places where calls to require that need the cwd are only within prototype functions, therefore only after you have an instance. Placing it outside the construct could affect calls to required modules that don't need to be in the cwd, normal mocha dependencies.
Right now this would affect only UIs and reporters, and they are in function prototypes.

Catch empty throws

giggio · 2014-07-05T05:16:13Z

Can we get some love in this pull request from the maintainers? It is a simple change. Some feedback, please?

…put bugs" * shaine/patch-1: Fix dot reporter output bugs

closes #1228

Show also failed test for doc reporter

Fix #1191 - Tests succeed on rejected falsey promises

xunit reporter: don't include attrs in failure tag

jbnicolai · 2014-07-11T23:55:01Z

The current way dependencies are handled forces us to have a ui interface (or reporter) installed globally.

I don't quite understand. For instance, the teamcity reporter simply works through npm install --save-dev mocha-teamcity-reporter, no need to install anything globally.

Add ES6 arrow function support to utils.clean

as mentioned in #332

giggio · 2014-07-12T04:19:17Z

@jbnicolai
You are correct. And this is because reporters are setup differently from uis. I just found this out, as your point made me curious. This is the difference I found out:
At this line we have, in the _mocha file, a setup of a reporter function, done through a require call, _in that mocha file. That _mocha file has module.paths setup right here in a way that pushes the cwd into it. The reporter function on mocha.js takes an argument that can be a string or a function, and when it gets to the point to use the reporter, it is already a function, locally loaded. The ui function, on the other hand, only take strings, this is why I made this PR the way it is, to be able to locally load it. I thought it would also fix reporters, but I was wrong, it only fixes UIs, as reporters already work, while local UIs do not.
An alternative approach to allow UIs to load local npm modules would be to setup UIs the same way Reporters are setup, so that they are locally loadable as well (updating the _mocha file to load the ui, and the mocha.js file to take functions on the ui method just like the reporter method. If the maintainers find this a better approach, please close this PR and let me know, and I will submit a new PR with that approach. It should be quick.

jbnicolai · 2014-07-12T10:23:40Z

@giggio many thanks for figuring out why reporters work and UIs do not.

I do feel that a consistent approach to loading in third party UIs and reporters would be best, so I'm closing this PR and am looking forward to one doing just that. Your work on this is greatly appreciated! :)

giggio · 2014-07-12T22:58:06Z

@jbnicolai
I was working on it just now and just figured out that actually reporters don't really work with locally installed packages either.
You can try it yourself. I used mocha-teamcity-reporter as you suggested, if you try to run
mocha test/unit/sample.coffee -R mocha-teamcity-reporter
You get:

/home/giovanni/.nvm/v0.10.26/lib/node_modules/mocha/lib/mocha.js:137
    if (!_reporter) throw new Error('invalid reporter "' + reporter + '"');
                          ^
Error: invalid reporter "mocha-teamcity-reporter"
    at Mocha.reporter (/home/giovanni/.nvm/v0.10.26/lib/node_modules/mocha/lib/mocha.js:137:27)
    at Object.<anonymous> (/home/giovanni/.nvm/v0.10.26/lib/node_modules/mocha/bin/_mocha:191:7)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:902:3

I incorrectly assumed on my earlier comment that the require call on _mocha was doing that. Actually, it isn't, and the code is weird. It would load the reporter after calling mocha.reporter(program.reporter);, but it never gets to that point, as it fails earlier on that very line. And if it ever gets to that point, to load the reporter with the require call on _mocha, it is not passed on to the mocha object, as the call to mocha.reporter is made earlier.
So, I don't know how Teamcity's plugin could possibly work while being a locally installed package. It doesn't work with me. Would you please test and report if it works for you and that you do not have it installed globally?

Now I see 2 options:

we can go ahead and use this PR I created.
we could change _mocha and get it to load this reporters and UIs before calling mocha.reporter and mocha.ui, and pass along the found functions if any.

Let me know what you prefer.

jbnicolai · 2014-07-12T23:04:02Z

@giggio again, your work is appreciated - thanks!

I have not tried the teamcity reporter personally, but I have recently experimented with moving out other reporters to their own modules. What I found there was that extracting a reporter to a separate module, installing that module through npm install or npm link and running mocha with ./bin/mocha --reporter=<reporter name> worked as expected.

I will look into the issues you're having, try teamcity locally as well, and get back to you.

jbnicolai · 2014-07-12T23:20:58Z

Okay.. so after some intense debugging I think the following happened:

I closed the pull request 13 hours ago
you rebased the pull request and force-pushed to update. However: you made a mistake, and did a hard reset to origin/master
I reopened the pull request, because you convinced me this was not a closed issue after all
Github recognized that the difference between your forks master and the original master was nonexistent. It therefore automatically marked this pull request as resolved.

So even though this pr is marked as merged, I did not actually merge in any of your changes. Had me worried there, for a moment 😉

giggio · 2014-07-12T23:25:43Z

Right, this PR is broken now. I thought github would be smarter and keep the files in place.

jbnicolai · 2014-07-12T23:28:09Z

@giggio unless my understanding is wrong I think you did something along the lines of git reset --hard origin/master && git push -f myfork master - thereby overwriting the pull request. In the future you'd want to use rebase rather than reset.

Feel free to recreate the PR and reference this one for context - I'm still more than open to accepting it if I can understand the issue and reproduce it locally.

giggio · 2014-07-12T23:28:19Z

Ok, this is all pretty weird now. I am not able to use locally installed reporters, I always get that error, "invalid reporter".
Maybe npm link works differently from npm install? Can you try to use install instead of link and try it out? You can try the mocha-teamcity-reporter, it will execute and throw.

jbnicolai · 2014-07-12T23:31:54Z

npm link <pkg> and npm install <pkg> work exactly the same, other than the package being linked to a local version in the first case, and downloaded from the registry in the latter.

It seems mocha-teamcity-reporter works just fine for me though.

giggio · 2014-07-12T23:33:41Z

That is because you are running from ./.bin/mocha. Try to run from global mocha. It will fail.

giggio · 2014-07-12T23:37:57Z

That is the problem I am trying to solve. If you run from the globally installed mocha, mocha will not be able to find your reporter/ui, because they are installed locally. If you run from the locally installed it will. We have two different behaviors, depending where you run it from.
I guess that if you run it with grunt or gulp it should work as well, as they always load the local mocha package.
I usually just type mocha <filename>. This use case does not work.

I will open another PR now. I created another branch so this mess doesn't happen anymore. (Mental note, never do a PR from master). :)

jbnicolai · 2014-07-12T23:40:48Z

./bin/mocha actually ;-) I was inside the mocha repository.

Right, now I understand the issue. I do think the current behaviour makes some sense though: if you're running mocha globally, use globally installed reporters; if running locally, use the local ones.

On the other hand, I see your point. When developing, you simply type mocha and expect it to work. Seeing as this shouldn't be able to break anything, I guess I'm happy to accept your PR 👍

giggio · 2014-07-12T23:45:21Z

\o/

claudyus and others added 5 commits January 26, 2014 18:34

report also failed test for doc reporter

f70a0b4

Add ES6 arrow function support to utils.clean

4e92dc6

Fix dot reporter output bugs

2b592a6

Fixed: - Pending test dots not being counted (leading to longer rows of dots when containing pending tests) - Off-by-one error on first dot row (first row always lacks a dot)

Catch empty throws

efc53eb

Fix #1191

114498b

Make sure that a test case returning a promise fails if the promise is rejected without a reason.

giggio added a commit to giggio/mocha-retry that referenced this pull request Jun 17, 2014

try to use local node module

8b5a820

This depends on a PR made on the mocha project: mochajs/mocha#1240

jsdevel reviewed Jun 19, 2014
View reviewed changes

jsdevel and others added 5 commits June 19, 2014 10:47

Honoring timeout for sync operations.

6f912b8

Fix tests to be faster.

9db4987

Add --watch-extensions to set extensions to watch

a886ed9

Merge pull request #1219 from LinusU/patch-1

c2c69a0

Catch empty throws

Adds maintainers to package.json.

ae55cbd

Joshua Appelman and others added 6 commits July 6, 2014 03:23

Updates to growl 1.8.0, gets rid of missing repository field warning.

c5525dc

"Merge pull request #1204 from shaine/patch-1\n\nFix dot reporter out…

cee700e

…put bugs" * shaine/patch-1: Fix dot reporter output bugs

Add --*-deprecation node flags (close #1217)

98a5bab

reporter: change default to spec

aaddbc6

closes #1228

change the default reporter in mocha(1) too duh

d2398cc

Merge pull request #1117 from claudyus/master

7ac4f01

Show also failed test for doc reporter

giggio mentioned this pull request Jul 8, 2014

Error Message Object has no method call giggio/mocha-retry#1

Closed

jonathanong and others added 2 commits July 8, 2014 15:56

Merge pull request #1224 from mattias-lw/fix_issue_1191

d5d1371

Fix #1191 - Tests succeed on rejected falsey promises

Merge pull request #1244 from oker1/patch-1

1ea878d

xunit reporter: don't include attrs in failure tag

Joshua Appelman and others added 2 commits July 12, 2014 01:58

Merge pull request #1176 from fitzgen/utils-clean-es6-arrows

85d1c60

Add ES6 arrow function support to utils.clean

updating package.json to reflect current maintainers

913964d

as mentioned in #332

giggio changed the title ~~Allow local modules to work as ui and reporters~~ Allow local modules to work as UIs Jul 12, 2014

jbnicolai closed this Jul 12, 2014

jbnicolai reopened this Jul 12, 2014

jbnicolai merged commit 913964d into mochajs:master Jul 12, 2014

giggio mentioned this pull request Jul 12, 2014

Allow local modules to work as UIs and Reporters #1267

Merged

snyk-bot mentioned this pull request Mar 12, 2020

[Snyk] Fix for 1 vulnerabilities Deckhandfirststar01/mocha#16

Open

snyk-bot mentioned this pull request Mar 29, 2021

[Snyk] Security upgrade nyc from 11.9.0 to 15.0.0 Deckhandfirststar01/mocha#30

Open

snyk-bot mentioned this pull request Sep 16, 2021

[Snyk] Fix for 1 vulnerabilities Deckhandfirststar01/mocha#34

Open

snyk-bot mentioned this pull request Jan 1, 2022

[Snyk] Security upgrade nyc from 13.3.0 to 15.0.0 Deckhandfirststar01/mocha#35

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow local modules to work as UIs #1240

Allow local modules to work as UIs #1240

giggio commented Jun 17, 2014

giggio commented Jun 17, 2014

jsdevel Jun 19, 2014

giggio Jun 19, 2014

jsdevel Jun 19, 2014

giggio Jun 19, 2014

giggio commented Jul 5, 2014

jbnicolai commented Jul 11, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

Allow local modules to work as UIs #1240

Allow local modules to work as UIs #1240

Conversation

giggio commented Jun 17, 2014

giggio commented Jun 17, 2014

jsdevel Jun 19, 2014

Choose a reason for hiding this comment

giggio Jun 19, 2014

Choose a reason for hiding this comment

jsdevel Jun 19, 2014

Choose a reason for hiding this comment

giggio Jun 19, 2014

Choose a reason for hiding this comment

giggio commented Jul 5, 2014

jbnicolai commented Jul 11, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014

giggio commented Jul 12, 2014

jbnicolai commented Jul 12, 2014

giggio commented Jul 12, 2014