feat(interfaces/*): support untitled suites/tests #1632 #1634

a8m · 2015-03-30T06:32:05Z

Further to issue #1632, I created this draft.
let's discuss about it.

https://travis-ci.org/mochajs/mocha/jobs/38422941 The only problem in this error is that npm v1.2 doesn’t support `^` version specifier. It's not the problem of Node v0.8 itself. So I updated the installation command to install the latest version of npm, before installing the dependencies of mocha.

Users may register `Runnable`s as asynchronous in one of two ways: - Via callback (by defining the body function to have an arity of one) - Via promise (by returning a Promise object from the body function) When both a callback function is specified *and* a Promise object is returned, the `Runnable`'s resolution condition is ambiguous. Practically speaking, users are most likely to make this mistake as they transition between asynchronous styles. Currently, Mocha silently prefers the callback amd ignores the Promise object. Update the implementation of the `Runnable` class to fail immediately when the test resolution method is over-specified in this way.

# By Shinnosuke Watanabe # Via Shinnosuke Watanabe * 'travis' of https://github.com/shinnn/mocha: Require npm version which supports `^` specifier

…v3.0.0 # By Rob Loach # Via Rob Loach * 'update/glob' of https://github.com/RobLoach/mocha: Update glob to 4.0.6

This results in a slight change to the behavior of --async-only: instead of failing immediately, check to see if the test returned a promise (or otherwise failed) before complaining about not having a done callback.

travisjeffery · 2015-03-30T06:46:35Z

the other option would be to throw an error with a good message, that would probably result in better practices/readable tests imo

a8m · 2015-03-30T08:57:17Z

So there are two options:

use a fallback title - Untitled suite/test
throw an error with a usage message (it means to failing, and not run at all).
Please correct me if I get you wrong @travisjeffery

I prefer the second option, but the first one is more forgiving.
/cc @dasilvacontin

travisjeffery · 2015-03-30T09:06:36Z

yeah let's go with throwing an error, i prefer that too. better to be opinionated on having people do the right thing.

dasilvacontin · 2015-03-30T09:40:03Z

Yeah, I also prefer throwing an error. As @travisjeffery, said, leads to more readable code and enforces good practice, and it's probably what you wanted to do anyways.

a8m · 2015-03-30T17:28:49Z

ping

danielstjules · 2015-03-30T18:26:50Z

I like the solution. It supports the possibility of creating interfaces that accept empty titles for either suites, tests, or both as well.

a8m · 2015-03-30T18:29:31Z

Thanks @danielstjules.

a8m · 2015-03-30T18:32:20Z

I started to implement it on the interfaces side, but then I got the feeling of repeating myself.

danielstjules · 2015-03-30T18:35:22Z

Yea, updating it at the suite/test level is good, since any interfaces can just use an empty string for the name if they'd like to accept "anonymous specs" :)

dasilvacontin · 2015-03-30T19:00:30Z

Was is possible to use an object as a title before? That was later being converted to a string inside our code.

Because if that was possible before, this should be considered breaking. Or we should check for a .toString() or something.

a8m · 2015-03-30T19:10:30Z

Using object as suite/test-case title, returning this result (is just toString it):

suite
    ✓ [object Object]

this should be considered breaking.

fair enough @dasilvacontin , I added it to v3.0.0 milestone

danielstjules · 2015-03-30T19:10:36Z

$ cat example.js
it({foo: 'bar'}, function() {});

$ mocha example.js


  ✓ [object Object]

  1 passing (8ms)

$ cat example.js
it(12, function() {});

$ mocha example.js


  ✓ 12

  1 passing (8ms)

edit: ha, beat me to it by seconds

dasilvacontin · 2015-03-30T19:23:32Z

But what if you accept objects that have a .toString() or are not a function?

danielstjules · 2015-03-30T19:35:34Z

@dasilvacontin Because mocha currently works with functions, too.

$ cat example.js
it(function() {}, function() {});

$ mocha example.js


  ✓ function () {}

  1 passing (9ms)

I don't personally see this PR as breaking the API and requiring a new major release. Minor would be fine, and would probably be a good fit for 2.3.0 or 2.4.0. It's unspecified behavior. We shouldn't be concerned if someone is relying on passing objects or functions as the spec or suite titles, rather than strings. The public API documents that the title should be a string: https://github.com/mochajs/mocha/blob/master/lib/suite.js#L25 https://github.com/mochajs/mocha/blob/master/lib/suite.js#L43 https://github.com/mochajs/mocha/blob/master/lib/test.js#L16

dasilvacontin · 2015-03-30T20:10:35Z

I agree that this does not break the API, but it may break backwards compatibility. If you make people waste time, you are doing the same harm; you don't want someone's build to suddenly stop working and make them waste time tracking down the issue to fix it.

Related: janl/mustache.js#407

a8m · 2015-03-30T20:12:20Z

make them waste time tracking down the issue to fix it.

that's the reason we added this user-friendly message :)

dasilvacontin · 2015-03-30T20:14:57Z

that's the reason we added this user-friendly message :)

User-friendly message that breaks your build and makes you ~~correct hundreds of tests in the worst case scenario~~ waste hours tracking down the issue, searching which version of mocha broke BC, correct package.jsons with a version of mocha in which everything still works, and write an issue in mocha's repo telling us the wonderful morning he just had.*

a8m · 2015-03-30T20:33:17Z

We are not breaking the API, this is just a weakness that maybe users take advantage of it.

/cc @travisjeffery

danielstjules · 2015-03-30T21:18:36Z

http://symbolhound.com/?q=it%28%22 / http://symbolhound.com/?q=it%28%27 vs http://symbolhound.com/?q=it%28%7B / http://symbolhound.com/?q=it%28function (the only actual match is a typo) I wish I could find a search engine with better support for symbols lol Any suggestions?

So for:

it(function() {}, function() {
});
it({foo: bar}, function() {
});
it(-1, function() {
});

it's still undefined behavior, since the public API docblocks (#1634 (comment)) specify that title should be a string, and nowhere on http://mochajs.org/ do we mention non-string titles are supported.

@dasilvacontin I hear what you're saying about preserving BC, and agree for the most part, but the issue you highlighted in mustache.js seems to be of a different scope. That said, if a 3.0.0 release is in the works for the coming months, I'm not worried either way. :)

dasilvacontin · 2015-03-30T22:05:38Z

Those queries can't come up with the edge-cases I mentioned.

Still, I agree that the number of people who could be affected by this is very very low. And afaik, since we have some serious bugs in mocha (eg #534), I wouldn't bump to v3 solely for this reason, since not getting more bugfixes in v2 would probably out-harm the problems merging this PR could cause.

In the case we were soon bumping to v3, I'd include this for the next major.

boneskull · 2015-03-31T16:38:48Z

imo, If it throws exceptions where it didn't throw exceptions before, it's not backwards-compatible and needs to wait until a major.

* commit '7657cb11d960cf2cd8407b256019b2e34dc93328': Allow --async-only to be satisfied by returning a promise

a8m · 2015-04-01T20:09:06Z

it's not backwards-compatible and needs to wait until a major.

fair enough.

closes mochajs#1644

Throw an exception when timeout too large.

* commit '3b02d830c0c5f20c5be9acaa9ef45b824bcbf965': (29 commits) Add cross-frame compatible Error checking for fail Remove moot `version` property from bower.json HISTORY: fix typo in 2.2.5 HISTORY: improve 2.2.5 changelog removing duplicate flags adding additional iojs flags Prevent default browser behavior for failure/pass links Removes return statement irt mochajs#1700. Removes accidentally commited test.js Add support of --harmony_arrow_functions V8 option Release 2.2.5 Upgrade jsdiff to v1.4.0 fix 'location is not defined' error Update json-stream.js Sanity check: update fixtures/regression/issue-1327.js to be closer to orig test Fix diff colors use a valid SPDX license identifier Add integration tests Handling of error.htmlMessage in the HTML reporter Split message and stack into two separate variables fix(utils/stringify): fix issue mochajs#1660 ... Conflicts: test/acceptance/misc/asyncOnly.js

boneskull · 2015-07-10T04:19:18Z

@a8m this needs rebasing against v3.0.0 as well

* master: (27 commits) Remove TODO from Browserify transition Build using Browserify Rework hook error tests to actually assert Move hook error test to integration in prep for rewrite Fix 1766: stackfilter should not ignore node_modules Remove __proto__ parsing from browser build scripts Replace __proto__ with lodash.create Removes heading newline. support escaped spaces in cli options Fixes indentation. Simplifies split regex, the filter already catches empty args. Removes unneeded trim, the filter does the same. Simplifies filter by truthy values. add lint check to test-all target. YES! remove dupe in contributors list lock down supports-color dependency lint runner.js Escape test/suite title for re in html reporter Remove npm version from engines field CI: Update npm when < 1.3.7 ... Conflicts: .travis.yml lib/runnable.js

Throw a user-friendly error when the suite title or the test-case title isn't provided.

a8m · 2015-07-18T14:45:20Z

Replaced with #1809(rebased on top of v3.0.0)

RobLoach and others added 6 commits October 19, 2014 12:56

Update glob to 4.0.6

3fff52e

Merge branch 'travis' of https://github.com/shinnn/mocha into v3.0.0

ad3057d

# By Shinnosuke Watanabe # Via Shinnosuke Watanabe * 'travis' of https://github.com/shinnn/mocha: Require npm version which supports `^` specifier

Merge branch 'update/glob' of https://github.com/RobLoach/mocha into …

17cec5b

…v3.0.0 # By Rob Loach # Via Rob Loach * 'update/glob' of https://github.com/RobLoach/mocha: Update glob to 4.0.6

Allow --async-only to be satisfied by returning a promise

7657cb1

This results in a slight change to the behavior of --async-only: instead of failing immediately, check to see if the test returned a promise (or otherwise failed) before complaining about not having a done callback.

a8m added this to the v3.0.0 milestone Mar 30, 2015

a8m force-pushed the untitled-suites-tests-1632 branch 2 times, most recently from bb2fef5 to 2d7f431 Compare March 30, 2015 15:59

Merge commit '7657cb11d960cf2cd8407b256019b2e34dc93328' into v2.3.0

7be4891

* commit '7657cb11d960cf2cd8407b256019b2e34dc93328': Allow --async-only to be satisfied by returning a promise

a8m added the TO-MERGE label Apr 1, 2015

callumacrae and others added 3 commits April 13, 2015 19:41

Throw an exception when timeout too large.

ace073d

closes mochajs#1644

Merge pull request mochajs#1652 from callumacrae/timeout-exception

450ecbf

Throw an exception when timeout too large.

Merge remote-tracking branch 'origin/v2.3.0' into v3.0.0

cc3f3f9

a8m mentioned this pull request Apr 14, 2015

feature(mocha/grep): improve grep - issue #808 #1650

Closed

jbnicolai force-pushed the master branch 3 times, most recently from 2f458ab to 2952eca Compare July 5, 2015 10:25

boneskull added the pr-needs-work label Jul 10, 2015

boneskull and others added 2 commits July 11, 2015 21:31

fix(Suite/Test): untitled suite/test-case mochajs#1632

710529e

Throw a user-friendly error when the suite title or the test-case title isn't provided.

a8m force-pushed the untitled-suites-tests-1632 branch from 2d7f431 to 710529e Compare July 18, 2015 14:30

a8m mentioned this pull request Jul 18, 2015

fix(Suite/Test): untitled suite/test-case #1632 #1809

Closed

a8m closed this Jul 18, 2015

boneskull removed the pr-needs-work label Dec 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(interfaces/*): support untitled suites/tests #1632 #1634

feat(interfaces/*): support untitled suites/tests #1632 #1634

a8m commented Mar 30, 2015

travisjeffery commented Mar 30, 2015

a8m commented Mar 30, 2015

travisjeffery commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

a8m commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

boneskull commented Mar 31, 2015

a8m commented Apr 1, 2015

boneskull commented Jul 10, 2015

a8m commented Jul 18, 2015

feat(interfaces/*): support untitled suites/tests #1632 #1634

feat(interfaces/*): support untitled suites/tests #1632 #1634

Conversation

a8m commented Mar 30, 2015

travisjeffery commented Mar 30, 2015

a8m commented Mar 30, 2015

travisjeffery commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

a8m commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

a8m commented Mar 30, 2015

danielstjules commented Mar 30, 2015

dasilvacontin commented Mar 30, 2015

boneskull commented Mar 31, 2015

a8m commented Apr 1, 2015

boneskull commented Jul 10, 2015

a8m commented Jul 18, 2015