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

feat(runner): fix '.only()' exclusive feature, #1481 #1807

Closed
wants to merge 13 commits into from

Conversation

a8m
Copy link
Contributor

@a8m a8m commented Jul 18, 2015

This PR replace #1591 plus I added some integration-tests for it(thanks @danielstjules)
IMO we should merge it to v3.0.0 asap, instead of rebase-it on master every time.

/cc @mochajs/mocha

Review on Reviewable

RobLoach and others added 12 commits October 19, 2014 12:56
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.
* commit '7657cb11d960cf2cd8407b256019b2e34dc93328':
  Allow --async-only to be satisfied by returning a promise
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
* 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
@a8m a8m added this to the v3.0.0 milestone Jul 18, 2015
@@ -117,6 +117,22 @@ test-only:
--ui qunit \
test/acceptance/misc/only/qunit

test-global-only:
Copy link
Contributor

Choose a reason for hiding this comment

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

@a8m these paths are incorrect

@boneskull
Copy link
Contributor

@a8m after correcting that, please merge. thanks!!

@dasilvacontin
Copy link
Contributor

I just noticed that there's duplicate code in the interfaces that could be refactored into Suite#setOnly(flag) and Test#setOnly(flag) (or similar). It would made the changes easier to understand and maintain.

@a8m
Copy link
Contributor Author

a8m commented Nov 12, 2015

PTAL @boneskull
@dasilvacontin can you please be more specific ? thx

@dasilvacontin
Copy link
Contributor

Welcome back @a8m! 😄

mocha.options.hasOnly = true;
suite.isOnly = true;
suite.onlyTests = (suite.onlyTests || []).concat(test);

and

mocha.options.hasOnly = true;
suite.isOnly = true;

are repeated multiple times through the code.

I was proposing refactoring the first one into Suite#addOnlyTest. And the second one into Suite#only (similar to the setters/getters Suite#timeout, Suite#slow, Suite#bail, etc)

(and Suite#addOnlyTest would internally execute Suite#only, besides adding the test to the array, ofc)

@dallonf
Copy link

dallonf commented Jan 13, 2016

What's still missing in this PR? Is it something relatively simple that I could finish?

Also, is this meant to fix the issue where describe.only overrides any it.only calls? (This is where I wound up when searching for that issue) Example:

describe.only("These should run", function() {
  it("this one should not run (but does)", function() { });
  it.only("this one should run", function() { });
});

describe("These should not run", function() {
  it("and of course don't", function() {});
  it.only("although I wonder about this one...?", function() {});
});

@boneskull
Copy link
Contributor

@a8m What is the status of this? @danielstjules / @dasilvacontin Either of you guys know?

@dasilvacontin
Copy link
Contributor

@boneskull I believe it was lacking follow up of my comments from @a8m.

@a8m a8m force-pushed the fix-exclusive-feature-1481 branch from fa1eb61 to 55ea1bb Compare March 9, 2016 12:57
@a8m
Copy link
Contributor Author

a8m commented Mar 9, 2016

Hi guys! sorry for this huge delay :/

I made some changes according to @dasilvacontin's comments; would you mind to take a look at it?
/cc @boneskull @dasilvacontin

This PR fix mochajs#1481, and also extends the .only() behaviour.
(i.e: it's not use grep anymore, support suite, test-case or both, add
the ability to run multiple .only)
@a8m a8m force-pushed the fix-exclusive-feature-1481 branch from 55ea1bb to 385b391 Compare March 9, 2016 13:16
@boneskull boneskull force-pushed the v3.0.0 branch 3 times, most recently from c8482d5 to 29a0de2 Compare July 2, 2016 23:07
@boneskull
Copy link
Contributor

merged into v3.0.0 branch via 8a75434

@a8m thanks!

@boneskull boneskull closed this Jul 3, 2016
@a8m
Copy link
Contributor Author

a8m commented Jul 3, 2016

Awesome!! thanks @boneskull

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.

9 participants