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

opt-out of globals #956

Open
domenic opened this Issue Aug 20, 2013 · 38 comments

Comments

Projects
None yet
@domenic
Copy link
Contributor

domenic commented Aug 20, 2013

@isaacs is complaining that Mocha tests aren't node-able. I think this is a silly complaint, but I think he represents a vocal minority, so shutting them up would be possibly valuable.

Here is what I envision: instead of being able to use e.g. describe, it, etc. by default, you'd do

var mochaBDD = require("mocha/bdd");
var describe = mochaBDD.describe;
var it = mochaBDD.it;

// ok, stupid boilerplate is over but at least I don't have to use a test runner, woohoo!

Alternately it could be just

require("mocha/bdd");

and that would set up some globals for you.

What do you think?

@isaacs

This comment has been minimized.

Copy link

isaacs commented Aug 21, 2013

Even if it just dumped its crap all over the global space when you require('mocha'), would be better than the current state of things.

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Aug 21, 2013

I don't think it's silly but there are tradeoffs with everything. I'd be happy with something like this:

var Mocha = require('mocha');
var mocha = new Mocha(options);
mocha.describe('blah blah', function....

no one would use it but at least it would be a cleaner way to implement what we currently have. There would be a ton of boilerplate that everyone would have to set up each time, but if we could narrow those down to CLI-ish APIs that would be ok. Even if there was lib/cli.js that just passed in the ARGV, but I still doubt anyone would use it, you can use it without the CLI reasonably easy, but that illustrates that no one really wants to beyond some edge cases.

@domenic

This comment has been minimized.

Copy link
Contributor

domenic commented Aug 21, 2013

@visionmedia that seems pretty nice. The reason I suggested require("mocha/bdd") or similar is that it would be pretty easy to implement in terms of existing Mocha, but yeah, yours is probably better. (You could envision using it to e.g. run multiple test suites at once or something. Well, that would probably break because of the process.on('uncaughtException') usage, but you see what I mean.)

I may try a pull request for this one day.

@park9140

This comment has been minimized.

Copy link
Contributor

park9140 commented Aug 21, 2013

This is a great example where a little future JavaScript via destructuring assignment goes a long way.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7#Destructuring_assignment_%28Merge_into_own_page.2Fsection%29

var [describe,it,beforeEach,afterEach] = new require("mocha")(options);

Just wish there was node harmony support for this.

@glenjamin

This comment has been minimized.

Copy link
Contributor

glenjamin commented Dec 8, 2013

Is the idea of this to be able to do

node test/a-mocha-test.js

And have it run that test?

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Dec 8, 2013

@glenjamin

This comment has been minimized.

Copy link
Contributor

glenjamin commented Dec 8, 2013

Having tried to do this myself on nodespec, the problem wasn't really making the various functions available, the problem was figuring out how/when to begin executing - we'd still need a two-pass define/run approach.

The options I came up with were:

  1. have every user add something like mocha.exec() to the bottom of every file they might want to run in isolation
  2. Wait for core to add something like process.on('exit'), but when the event loop is still available
  3. assume each file has only one top-level describe block, and begin execution when that finishes

(1) is probably the nicest, which could look like this:

var run = require('mocha/required');

describe('blah blah' , function() {
// ..
});

run();

This doesn't seem to add much over doing node ./node_modules/.bin/_mocha test/a-mocha-test.js when you really need to run without the mocha wrapper.

@jbnicolai

This comment has been minimized.

Copy link
Contributor

jbnicolai commented Jul 5, 2015

I think this no longer fits with how mocha currently is used. As this thread has been inactive for over a year, I'm closing it for now.

If anyone is still interested in this, feel free to comment or propose a pull-request and we'll consider :)

@jbnicolai jbnicolai closed this Jul 5, 2015

@jfirebaugh

This comment has been minimized.

Copy link
Contributor

jfirebaugh commented Sep 11, 2015

Could this be reopened? I'm still very much interested in it. Not being able to just node test.js mocha tests was one of the main reasons me and my colleagues at Mapbox have been moving away from mocha to harnesses like tape and tap. Personally I'd prefer to stick with mocha but do find that "node-ability" argument somewhat compelling.

Edit: the "node-ability" argument was elaborated by @tmcw here.

@danielstjules danielstjules reopened this Sep 11, 2015

@jfirebaugh

This comment has been minimized.

Copy link
Contributor

jfirebaugh commented Sep 11, 2015

For clarity, the lack of a requireable interface is not the primary blocker here from a user perspective. The following is already documented to work:

var describe = require('mocha').describe;
var before = require('mocha').before;
var it = require('mocha').it;

And it gets nicer with ES6:

import { describe, before, it } from 'mocha';

The issue is the fact that this only works when running via the mocha binary.

@danielstjules

This comment has been minimized.

Copy link
Contributor

danielstjules commented Sep 11, 2015

Oh, thanks for the clarification. :) If we already offer this ability when ran using the mocha binary, I agree it'd be nice to do the same with node. Will look into it. Thanks!

@glenjamin

This comment has been minimized.

Copy link
Contributor

glenjamin commented Sep 12, 2015

Now that node has a beforeExit hook, this seems fairly doable.

Would need to be something like

var { describe, it } = require('mocha/auto')
@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Sep 12, 2015

Yeah. As TJ said, the problem is even if we were to make everything "node-able", it may require too much boilerplate to be useful.

However...

@glenjamin thanks for the heads-up on beforeExit. I think we could leverage this. People would probably still complain it's too much "magic"...

boneskull added a commit to boneskull/mocha that referenced this issue Sep 12, 2015

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Sep 12, 2015

I've added a proof-of-concept to 3cdd4a0

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Sep 12, 2015

anyway, it would need a bit of work, but I think we can easily generalize it for all interfaces. there is no support here for Growl, mocha.opts or various other process-related stuff like exit codes. We should extract some code from bin/_mocha and reuse it here. Then we could have argument support as well. For example:

$ node test/my-test.js --reporter=dot

If anyone has any suggestions, lemme know

@glenjamin

This comment has been minimized.

Copy link
Contributor

glenjamin commented Sep 12, 2015

I think extracting some of the stuff in bin/_mocha into some sort of CLI would be pretty sensible regardless.

I'm still not sure this is really a useful addition though?

Is there much of a practical difference between any of the following?

node test/test.js
./node_modules/.bin/mocha test/test.js
PATH=./node_modules/.bin:$PATH mocha test/test.js
npm test -- test/test.js

I get the feeling that people who aren't using mocha because they can't run test files via node are simply people who don't like mocha, and are looking for an excuse! Mocha doesn't have to be for everyone 😄

@screendriver

This comment has been minimized.

Copy link

screendriver commented Dec 8, 2015

Same here. I am using jspm and SystemJS and can't import Mocha in my tests that are running at the browser.

import { describe, before, it } from 'mocha';

Can't be used when used in browsers.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Dec 9, 2015

@glenjamin it kind of just needs to happen. Mocha should have a core programmatic API. A nice side-effect of that is node-able tests. The environment in which it runs will consume the programmatic API, whether that's a browser, CLI, or something else entirely. Mocha.app, anyone? :)

@kuraga

This comment has been minimized.

Copy link

kuraga commented Dec 22, 2015

Interested of this feature!

@jadbox

This comment has been minimized.

Copy link

jadbox commented Dec 28, 2015

+1 for:
import { describe, before, it } from 'mocha';

alexanderchr added a commit to alexanderchr/isomorphic-style-loader that referenced this issue May 4, 2016

@drazisil drazisil added the feature label Mar 30, 2017

mheiber added a commit to nymag/clay-space-edit that referenced this issue May 16, 2017

test WIP
Ripped out Mocha because it doesn't let you import `describe`
mochajs/mocha#956 and mocha-loader
didn't work and come on, they should export `describe`.

mheiber added a commit to nymag/clay-space-edit that referenced this issue May 16, 2017

test WIP
Ripped out Mocha because it doesn't let you import `describe`
mochajs/mocha#956 and mocha-loader
didn't work and come on, they should export `describe`.
@stevenvachon

This comment has been minimized.

Copy link

stevenvachon commented Oct 6, 2017

We currently have const {describe, it} = require('mocha'), but it isn't necessary because the globals already exist.

If we're concerned with browser support, we could always do const {describe, it} = window.mocha. We already have to do this for chai.

@mochajs mochajs deleted a comment from aristov Oct 6, 2017

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Oct 6, 2017

well, const {describe, it} = window too, I suppose.

this is so fundamental to mocha's architecture that mocha exports the global object when bundled.

@stevenvachon

This comment has been minimized.

Copy link

stevenvachon commented Oct 6, 2017

I don't think that mocha should define any global other than a single namespace, and maybe only for browsers.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Oct 6, 2017

the mocha executable will continue to use globals; that will likely never change.

what we can (and should) shoot for is the ability to easily run mocha tests via node without polluting the global namespace. that’s what this issue is about.

unfortunately, that’s a Texas-sized ball of twine nobody has had the time or energy to unravel, which should be apparent by the age of this issue...

@stevenvachon

This comment has been minimized.

Copy link

stevenvachon commented Oct 6, 2017

the mocha executable will continue to use globals; that will likely never change.

While I know nothing about how mocha is structured, I can assert that this single design choice is the source of the problem.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Oct 6, 2017

likely so!

@ScottFreeCode

This comment has been minimized.

Copy link
Member

ScottFreeCode commented Oct 7, 2017

I actually took a second look at the code, figured out (more or less) what I'd done wrong last time I'd tried to hack around having globals, thought about what it would take to get test files runnable with node test/someFile.js, and believe I know how to do both of them -- with the caveat that doing so without tripling the number of edge cases is likely to require breaking some existing edge cases. In more detail, my thoughts on the design are:

  • avoiding globals
    • Even in a semver major we cannot break the overwhelming majority of Mocha usage that uses the globals. The team could all quit their day jobs, never sleep again, spend the rest of their careers on it and still literally not have enough man-hours to handle the unimaginable flood of support requests that would entail. Therefore: this will be behind a flag, forever.
    • Currently, the BDD and TDD interfaces are special-cased: both will be exported (and no other interfaces) as require("mocha").it et al. (Note: the reason this doesn't break when other interfaces are selected may be that Mocha has a bug where under most circumstances it will set up the BDD interface even if another is selected. #2207) I want to move away from this.
      • Custom (third-party) interfaces should be able to work with the exporting scheme (as long as they use the emitted context object instead of hardcoding global).
      • We can't have every possible interface adding its functions to Mocha (what require("mocha") exports).
      • If we ever fix the bug where the BDD interface is always set up, it's going to be tricky to also keep exporting the BDD and TDD interfaces on Mocha another interface is chosen.
      • You shouldn't be setting Mocha's UI to TDD and using the BDD interface or vice versa anyway, unless maybe you do require("mocha/tddInterface").test or require("mocha/interface/bdd").it (not to be confused with "mocha/lib/interfaces/<interface name>" where the implementations live).
      • We could maybe keep the export-BDD-and-TDD-on-Mocha-if-possible behavior for backwards compatibility until a semver-major. I don't expect a lot of people are going to complain when we do drop it given that the people who requested and are using it know more about what they're doing than most and aren't entirely satisfied by it anyway (since the globals are still there).
  • node-ability, i.e. being able to run node test/someFile.js instead of mocha test/someFile.js
    • While it's less ubiquitous than the globals, I don't think we can afford to break people who are using var Mocha = require("mocha"); var mocha = new Mocha() etc. (i.e. the "programmatic interface"). We'd either have to detect that and avoid running Mocha "the node-able way" or else (easier for us, but requires opt-in from users) require that node-ability be accessed through a different import, e.g. require("mocha/nodeable").it (.describe etc.). A special import instead of piggybacking on require("mocha") would be easier to implement safely and would fit with my desire (described and justified in the avoiding globals section) to move away from exporting interfaces on the Mocha object exported by require("mocha").
    • To be able to node test/someFile.js, it's not enough that someFile.js be able to import Mocha's interface: Mocha has to be instantiated, and after the tests are all set up Mocha has to run. In other words, require("mocha/nodeable") must instantiate Mocha as well as exporting the interface, and then... after the test file runs it has to run Mocha.
      • One way to do this would be to run Mocha in process.on("beforeExit", provided the test file doesn't start any asynchronous actions.
      • Another way to do this would be to run Mocha in process.nextTick or setImmediate or setTimeout(..., 0 /* or some small number */).
      • Tests that set up their stuff with --delay and asynchronously run with run() would be a problem. Not for knowing when to run -- that's easier, there's run() -- and not even for knowing whether to --delay (see below, or provide require("mocha/nodeable/delayed")), but rather because run() is only supposed to be called in one test file as currently designed. Arguably that design is already hard to work with and we'd need to fix it so that run() should be called in every test file anyway, in which case using it with "node-able" tests would be easy after fixing it. Otherwise... I'm not sure what to do.
    • If we want to be able to allow calling these test files through mocha instead of node (i.e. using this doesn't force use of node), then whatever is required has to detect when the CLI is in use and not instantiate Mocha then. This would be easier than detecting when the programmatic API is in use -- the CLI can set some global state that these modules would check, or these modules can check whether the CLI is the file that Node ran.
    • We'd need to make the CLI pieces more modular if we wanted to support passing Mocha flags when running through Node, or using mocha.opts, or anything that would allow node-able test files to use behavior other than Mocha's default behavior.
  • Combining these two ideas' constraints, ***I propose (and believe I can implement)***:
    • avoiding globals
      • Instead of passing interfaces the global object to put their functions onto, it will pass them a "selected interface object". (EDITTED TO ADD: this has to be done in both lib/mocha.js and browser-entry.js. As far as I'm aware all global-related parts of the proposal are doable both in Node and the browser.)
      • Afterward, it will copy all the contents of the selected interface object into global.
      • A localInterface option (--local-interface on the CLI) will stop Mocha from copying from the selected interface object to global.
      • DEBATABLE: Initially, for backwards-compatibility with require("mocha").it Mocha will copy TDD/BDD functions from the selected interface object onto Mocha as well, but this will be deprecated/removed later.
        • (NOTE: if we stop BDD from being set up when it's not selected, we may have to do even more special handling here to set up the BDD on a dummy selected interface object from which these can be copied -- a good case for dropping the behavior altogether, it just gets more complex the more other things we fix/improve while still trying to support this.)
      • require("mocha/interface") will export the selected interface object.
      • require("mocha/interface/<specific interface>") will call the specified interface with a new selected interface object that it will subsequently export.
      • EDITTED TO ADD: require("mocha/interface") will also be a property on the mocha object in the browser, mocha.interface, to support browser usage without a module system.
    • node-ability
      • require("mocha/nodeable/<specific interface>") will instantiate Mocha, set up Mocha to run after the test file (presumably using setTimeout unless delay is used, see above re. delay) and export a selected interface object for that interface much like require("mocha/interface/<specific interface>")
      • If CLI options or mocha.opts is supported, require("mocha/nodeable") would instantiate Mocha, set up Mocha to run after the test file (see above), and perform the same interface setup steps as the CLI, exporting the selected interface object just like require("mocha/interface").
      • If we want node-able test files to also be mocha-able then mocha/nodeable & mocha/nodeable/<specific interface> isn't necessary, we can piggy-back the node-ability on mocha/interface & mocha/interface/<specific interface>.

Thoughts?

@ScottFreeCode

This comment has been minimized.

Copy link
Member

ScottFreeCode commented Oct 8, 2017

Actually, on further thought, I motion to close this ticket, on the following rationale against using node <test file> in place of mocha <test file>:

  • I'm not aware of any advantages it offers.
    • One possible idea is that it only runs a single test file regardless of what globs are in mocha.opts. Simple solution: put globs in the test script in package.json instead: npm test will run all of them and mocha (node_modules/.bin/mocha, npx mocha) will run only the specified test file(s). (Files or globs that should run for any test run regardless of how many test files are run should still go in mocha.opts.)
    • Another possible idea is that it allows bypassing mocha.opts altogether. This should be achievable now using mocha --opts /dev/null (or on Windows mocha --opts nul). If for some reason it isn't, it would be simpler to implement a CLI flag to skip mocha.opts processing.
  • It can be implemented outside Mocha; instead of "mocha/nodeable", just put the same code using Mocha's programmatic API into a hypothetical package "nodeable-mocha".
    • I'm not saying it would be easy to implement in the first place, I'm saying I'm not aware of any way it could be more difficult to implement due to being outside Mocha.

Should we discover any significant advantage that is easier to achieve in core than outside of it, we can always reopen.

However, I also motion to reopen the avoiding-globals ticket, because:

  • While the advantages are unlikely to be relevant (how many other libraries are using globals named describe, it etc.?), they're possible in principle (the answer to that question could be greater than 0, however few) -- not dumping stuff into the global namespace is, theoretically, the Right Thing to Do.
    • Furthermore, my plan would extend the same behavior to third-party interfaces that are correctly written to use the emitted "context" object (for which Mocha currently always uses the global object) rather than hardcoding the global object; while our own interfaces seem unlikely to interfere with other libraries, we can't make any guarantees about other interfaces, so we'd be making it easier for them to do the right thing too.
  • My plan would clean up a lot of related edge cases in the current behavior (e.g. extending it to third-party interfaces, but also consistency in Mocha instead of special-casing the BDD and TDD interfaces).
  • The only easy way to implement it outside Mocha is to use Mocha's programmatic API and monkey-patch it, and this would leave some of the aforementioned inconsistencies and edge cases too.
@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Oct 8, 2017

so you’re suggesting rather that Mocha provides a way to run tests with mocha but behind a flag that won’t pollute global?

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Oct 8, 2017

part of what makes mocha mocha and part of its success is that it doesn’t require boilerplate to import stuff. despite the dogma around polluting the global namespace there are completely valid reasons to do so for developer ergonomics.

@ScottFreeCode

This comment has been minimized.

Copy link
Member

ScottFreeCode commented Oct 8, 2017

so you’re suggesting rather that Mocha provides a way to run tests with mocha but behind a flag that won’t pollute global ?

Yup.

part of what makes mocha mocha and part of its success is that it doesn’t require boilerplate to import stuff. despite the dogma around polluting the global namespace there are completely valid reasons to do so for developer ergonomics.

I'm not saying I disagree in general, but as an opt-in feature, I can see where some developers may want imports instead of globals (maybe they want their module to name a local variable context without accidentally referring to the global if they forget var, maybe they'd like to use a third-party interface where that sort of collision is even more likely), and now that I've figured out how to do it right it's also easier to do in Mocha than as an add-on of some sort (in fact, if I don't get sucked into something else, I'll probably throw together a rough draft in a branch this week to demonstrate).

(Plus we're already maintaining a kinda-sorta attempt, where after the interface is set up on global it's copied onto the Mocha object so you can require("mocha").it, but the version we're already maintaining frankly is weird, inconsistent, bug-prone and doesn't even actually avoid polluting the global namespace. If we're going to keep that sort of feature at all, we may as well correct it rather than maintaining a dubious version.)

This is IMO in contrast to being able to node <test file> for Mocha tests, which as far as I'm aware doesn't have any advantage and would be equally hard to implement inside or outside Mocha.

@zapphyre

This comment has been minimized.

Copy link

zapphyre commented Oct 11, 2017

@stevenvachon
how come is that const {describe, it} = require('mocha') is not working for me? I just got undefineds...

$ node -v
v8.4.0
@stevenvachon

This comment has been minimized.

Copy link

stevenvachon commented Oct 11, 2017

@zapphyre you still need to run the test suite via the mocha program, not via node.

@boneskull boneskull changed the title node-able tests opt-out of globals Oct 17, 2017

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Oct 17, 2017

You can't run multiple source files with node, so you're stuck with mocha. I don't know of a test runner/framework that doesn't have its own executable.

Updated the issue title to be more specific

@lll000111

This comment has been minimized.

Copy link

lll000111 commented Nov 7, 2018

Another reason for this: I would like to run tests using a different JS runtime, for example low.js (because it's one of our platforms, others are node.js, browser - and React Native). I can do that when I have a JS file that imports mocha and runs the tests, I can't do that when I have to call mocha to run tests.

I mean, you already do it for the browser. On that platform I have JS code that imports "mocha" and then runs the tests from a global mocha object using mocha.run().

In the browser I

  • Load mocha and get a global mocha object
  • Run mocha.setup({ui: 'bdd'});
  • Load all test files using SystemJS
  • Then I run
      mocha.checkLeaks();
      mocha.globals([]);
      mocha.run();

And that's it. When I try the same in node.js, using a require statement instead of SystemJS to load a test file, I get (using mocha 5.2.0)

> mocha.run();
TypeError: Cannot read property 'search' of undefined
    at Mocha.mocha.run (/home/mha/one/node_modules/mocha/mocha.js:149:54)

EDIT: I tried https://github.com/mochajs/mocha/wiki/Using-Mocha-programmatically and mocha.addFile instead of just loading it, same result.

@protometa

This comment has been minimized.

Copy link

protometa commented Nov 7, 2018

Is there currently any way to import describe/it?

@lll000111

This comment has been minimized.

Copy link

lll000111 commented Nov 7, 2018

There is a new (as of now 20 hour old) Wiki page https://github.com/mochajs/mocha/wiki/Using-mocha-programmatically but that is for running the tests.

@glenjamin

This comment has been minimized.

Copy link
Contributor

glenjamin commented Nov 7, 2018

Another reason for this: I would like to run tests using a different JS runtime, for example low.js (because it's one of our platforms, others are node.js, browser - and React Native). I can do that when I have a JS file that imports mocha and runs the tests, I can't do that when I have to call mocha to run tests.

If the alternative runtime has a node-like executable (which I think low.js does?) then you can do alternative-executable ./node_modules/.bin/_mocha, and that should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment