qunitjs / qunit

🔮 An easy-to-use JavaScript unit testing framework.
https://qunitjs.com
MIT License
4.01k stars 780 forks source link

Deprecate `module`, rename to `group` #190

Closed jzaefferer closed 12 years ago

jzaefferer commented 12 years ago

module conflicts with the one defined by CommonJS.

Would need to deprecate module and introduce group as an alias, then figure out if and when we can actually remove module.

Should encourage mocha to also use group instead of suite, as that conflicts with our use of suite.

pimvdb commented 12 years ago

"Suite" may be a bit ambiguous since QUnit presents itself as "a powerful, easy-to-use, JavaScript test suite". I don't know of a good alternative, nor whether this really matters, but I think it's worth noting.

tj commented 12 years ago

"suite", "label", or "title" would be what I would go for

sindresorhus commented 12 years ago

"group", "bundle", "set", "collection"

jzaefferer commented 12 years ago

Any objections to group? That works as a test suite consists of groups of tests.

jzaefferer commented 12 years ago

Do we also need to rename moduleStart and moduleEnd callbacks?

JamesMGreene commented 12 years ago

I would say yes to renaming the lifecycle logging events too.

jzaefferer commented 12 years ago

Closing, as globals are optional: QUnit.module will always work.

The module definition in Harmony ( http://wiki.ecmascript.org/doku.php?id=harmony:modules ) doesn't conflict, as @rwldrn will also tell you.

rwaldron commented 12 years ago

The forthcoming native module implementation was carefully specified to allow all existing uses of the word "module" as an Identifier, it is parsed contextually so that it must match the following exactly:

ModuleDeclaration ::= "module" [NoNewline] Id "=" Path ";"
                   |  "module" [NoNewline] Id "{" ModuleBody "}"

Which won't conflict with any extant use of the word "module" :)

broofa commented 12 years ago

Reopen, please.

While I appreciate that Harmony modules won't be an issue, that doesn't change the fact that the 'module' object is a de-facto standard with CommonJS. For better or worse, Commonjs is going to be around for a while, and has a significant following thanks to node.js. (11.5K modules on npmjs.org at last count).

The emerging practice for these modules is to use typeof(module) != 'undefined' to determine what context the code is running in (for better or worse). Rather than forcing an unknown number of libraries to implement more convoluted checks, it seems logical to instead change QUnit so that it's not publishing a proprietary object to the 'module' global. No?

Related: https://github.com/broofa/node-uuid/issues/43 https://github.com/cjohansen/Sinon.JS/issues/91

JamesMGreene commented 12 years ago

With regard to @pimvdb's note of:

"Suite" may be a bit ambiguous since QUnit presents itself as "a powerful, easy-to-use, JavaScript test suite".

To me:

I really like @visionmedia's Mocha's TDD syntax, using nestable suite's. Not to mention its other hot features, like "pending" tests.

pimvdb commented 12 years ago

@JamesMGreene: I agree. Just thought I would mention it, since terminology changes are not very pretty if they introduce ambiguity.

jzaefferer commented 12 years ago

@broofa could you point me to where CommonJS specifies module? All I see is its exports definition, along with the non-specced module.exports convention in NodeJS.

broofa commented 12 years ago

@jzaefferer: http://wiki.commonjs.org/wiki/Modules/1.1 - Section 1.1, "Module Context".

jzaefferer commented 12 years ago

@broofa right, which doesn't specify module.exports, only exports and module, but not a exports property of module.

broofa commented 12 years ago

@jzaefferer - Why is that important? The main issue (at least my main issue) is QUnit's declaration of a 'module' global. The specific properties of that global aren't of particular interest.

jzaefferer commented 12 years ago

Reopening to discuss it again at the next testing team meeting - that would be tomorrow, if we get a few people together, see also calendar on http://jquery.org/meeting/

scottgonzalez commented 12 years ago

Only two examples have been given where a conflict occurred and both have been resolved already. Changing the name in order to avoid future problems that seem unlikely and are easily fixable seems silly when the cost is breaking all existing code that is using the module() function.

andrewchch commented 11 years ago

After being bitten by this issue a couple of times, I think it would at least be worth documenting it somewhere to potentially save others some time in the future.

JamesMGreene commented 11 years ago

@andrewchch: Can you share an example of how you are getting bitten?

Reviewing the current export code, the only pertinent export I see is basically setting the whole QUnit object (QUnit.constructor.prototype) as exports... it does not extend the global (so long as the exports object exists).

If so, then its usage would always be referencing whatever variable you required the module into rather than being a global (unless you choose to extend the global, which would be silly), e.g.:

var QUnit = require('qunitjs');
QUnit.module('blah');
QUnit.test('blah blah blah', function(assert) {
  assert.ok(true);
});