Fix .only() behaviour when running with watch option, fixes #2429 #2544

ddneat · 2016-10-16T15:11:15Z

Since version 3.0.0 the watch option does not work as expected in combination with .only().

@fediev already described in detail #2429 the misbehaviour.

boneskull · 2016-11-01T04:56:02Z

@davidspinat thanks. can you please sign the CLA?

ddneat · 2016-11-02T10:42:56Z

@boneskull of course 👍

ddneat · 2016-11-23T13:26:08Z

@boneskull may I help you with something else? :)

boneskull · 2016-11-24T01:53:22Z

@davidspinat Sorry, bandwidth problems.

This may not be feasible, but can you provide an integration test? If you're having trouble, I can dig deeper and suggest something.

Failing that, maybe you see if you can't re-create the problem via other means...

ddneat · 2017-01-02T15:08:13Z

@boneskull Sorry haven't managed to provide an integration test. However I would suggest the following:

In https://github.com/mochajs/mocha/blob/master/test/integration/helpers.js#L64 we might extend the signature of runMocha with the an argument called times e.g.:

runMocha: function (fixturePath, args, fn, times) ...

Further we would need to modify invokeMocha to rerun the tests with different arguments/files... Nevertheless I'm not sure if it's worth the effort. Seems quite overwhelming and confusing to me.

What's your opinion?

jzaefferer · 2017-01-11T11:33:39Z

For the record, I had the same issue as reported in #2429 and just tested this branch locally, it works and fixes issue. Not the same as automated tests, but considering the two changes relatively trivial, maybe its good enough.

ddneat · 2017-02-03T12:24:22Z

@boneskull how should we proceed here? :)

threepears · 2017-06-20T20:05:38Z

@boneskull : just curious if this is being, or has been, integrated yet. My team has been watching this page for months, waiting to update our Mocha version until this issue is fixed, as we found the problem with ".only()" to break behavior we had relied on. This ticket is still listed as open, but if you've integrated this into a recent version, please let us know. Thank you!

juank11memphis · 2017-07-01T16:50:56Z

Is this bug getting fixed any time soon?
Thanks!

ddneat · 2017-07-24T22:32:41Z

Is there any chance this PR will be merged? How might we add a test which covers this fix? I guess everyone would love to see this merged. Please leave some feedback.

Greetings

boneskull · 2017-08-01T23:04:33Z

@davidspinat Can you please revert the change to the root mocha.js? This file is generated (manually, currently) when we release.

boneskull · 2017-08-01T23:11:12Z

A proper harness for testing watch-related behavior would be necessary if we were to make enhancements in this subsystem (or re-implement it).

The risk here is that the change involves code that's run regardless of whether --watch is invoked. If someone (like @jzaefferer?) has been running against this branch for awhile, it'd help to know that it's not causing problems elsewhere.

boneskull · 2017-08-01T23:54:22Z

lib/runner.js

@@ -795,7 +795,7 @@ Runner.prototype.run = function (fn) {
  var rootSuite = this.suite;

  // If there is an `only` filter
-  if (this.hasOnly) {
+  if (hasOnly(rootSuite)) {


Okay, so it looks like with the addition of --forbid-only, this may need some changes.

Runner#hasOnly is set if e.g. a describe.only() is encountered (see lib/interfaces/common.js).

The --forbid-only functionality will check this value. At the point that it's checked, the Runner may or may not have a suite property. This means there's likely some weird interactions between --forbid-only and --watch.

A solution may be to set Runner#hasOnly to true if the result of hasOnly() is true. hasOnly() may also want to check the value of Runner#hasOnly. When the Runner's end event is handled, it may also need to reset Runner#hasOnly to false.

I haven't fiddled with this too much, but this is the matrix we're trying to assert is OK:

describe.only()

it.only()

--watch

--forbid-only

For example, run a --watch with --forbid-only, then add a describe.only(), save, then remove it again & save. What happens?

(careful with nested suites here; hasOnly() looks like it only inspects the innards of any given suite, and not if that suite was itself marked as an "only" suite; that information is stored in the aforemention suite's parent! 😄)

Worth noting that #2874 also touches hasOnly-using code in addition to the --forbid-only code already merged doing so.

(I plan on a more detailed review later this week.)

This should be merged correctly now, I think.

boneskull

will need further experimentation and/or tests

boneskull · 2017-08-01T23:57:42Z

lib/runner.js

@@ -795,7 +795,7 @@ Runner.prototype.run = function (fn) {
  var rootSuite = this.suite;

  // If there is an `only` filter
-  if (this.hasOnly) {
+  if (hasOnly(rootSuite)) {


(careful with nested suites here; hasOnly() looks like it only inspects the innards of any given suite, and not if that suite was itself marked as an "only" suite; that information is stored in the aforemention suite's parent! 😄)

wrong button

jzaefferer · 2017-08-02T08:35:36Z

The risk here is that the change involves code that's run regardless of whether --watch is invoked. If someone (like @jzaefferer?) has been running against this branch for awhile, it'd help to know that it's not causing problems elsewhere.

I can't say how long I was running this branch locally, might've been a few weeks though. I never noticed any regressions.

Looks like this will requires further changes now - let me know if I can help testing the result.

ScottFreeCode

(TL;DR? Recommendations at the bottom.)

on design and concerns

(careful with nested suites here; hasOnly() looks like it only inspects the innards of any given suite, and not if that suite was itself marked as an "only" suite; that information is stored in the aforemention suite's parent! 😄)

As long as it's used only on:

the root suite
inside the filterOnly function

...it should be fine. (And I believe that's the case here.) If we're really paranoid, we could make the existing hasOnly function only accessible to two other functions: filterOnly and a runner-level hasOnly function that calls this existing hasOnly against the root suite, the new runner-level hasOnly function being what we would use here and in the forbidding-related code (see also second section). Alternatively, again if we're really paranoid, maybe we could update the hasOnly function to also check suite.parent._onlySuites.indexOf(suite) >= 0 or something like that? But as long as stuff's tested I don't think either of those is a necessity (see more on that in the rest of this section).

A proper harness for testing watch-related behavior would be necessary if we were to make enhancements in this subsystem (or re-implement it).

If someone had come up with one by now I'd be all for using it, and it would be great to come up with one so we can start in on trying to deal with other watch-related behavior, but for this particular fix I'm not convinced it's worth holding up on something that hard to figure out -- given that the hasOnly function can be tested to be correct on any given run and merely won't get stale like the hasOnly property. And speaking of testing for correctness on any given run...

The risk here is that the change involves code that's run regardless of whether --watch is invoked.

...I'm not sure there's much risk involved in this regard since we already have tests for the behavior of only in general. Surely those cover it?

Of course, if somebody can come up with a test that doesn't use --watch but does run the tests twice with onlyd suites/tests removed the second time, to reproduce the same issue that this fixes, we should incorporate that test into this PR; but (again) I'm fairly confident in the correctness of this particular change without such tests -- as long as we don't break the tests we already have, we at least won't be getting worse than we are now.

on what is required of this PR (including intersection with forbidding)

As far as I can tell, effectively, any place that the hasOnly property is being used ought to be replaced with calling the hasOnly function against the root suite. (The hasOnly property is on the runner, so all such replacements would be equivalent to using the hasOnly function against the root suite, and are thus safe/correct.) Prior to the addition of the forbid options, this PR addressed the only use that I'm aware of; however the forbid options are now using it (and the additional PR #2874 moves that usage to achieve error reporting). That means a few things are going to need to happen:

This PR should be rebased so it doesn't leave hasOnly property usages in master that were added after this was initially devised. (That will also be a good opportunity to drop the changes to the bundled mocha.js file.)
This PR should additionally remove the place(s) where the hasOnly property is being set (lib/reporters/common.js and lib/mocha.js, not to be confused with the bundled mocha.js), so that any future changes to use of hasOnly (such as Print failures for pending/only forbidden #2874) will be detected to fail tests if they use the property rather than the function. (Note that I wanted this done sooner or later anyway because, as discussed in --watch option works strangely with .only() #2429, the root cause of the problem here is needing a single source of truth.)
Either this PR's rebase should wait till Print failures for pending/only forbidden #2874 lands, or Print failures for pending/only forbidden #2874 will have to be updated to use the hasOnly function after this PR lands. (I am comfortable personally taking responsibility to handle the update to Print failures for pending/only forbidden #2874 if need be.)

@boneskull Any serious concerns with those recommendations, given the rationale and responses to concerns laid out above?

ScottFreeCode · 2017-08-06T06:04:49Z

Waaait a second, I just realized something else: @mochajs/core is runner.hasOnly, the (potentially stale) property, meant to be public? Because if so:

We'd have to wait for a "major" release to remove it. (As previously stated, I believe it is ultimately correct to replace it with a function that checks the current testsuite so stateleness and any other source-of-truth problems cannot occur; so I would advocate for doing so eventually.)
Until then, we'd want to fix it being incorrect on subsequent runs for consumers of the runner and not just for this built-in behavior.

In other words, if runner.hasOnly is public, I think the right thing to do may be to table this PR in favor of #2934 or something like it, and instead use my recommendations (remove setting the hasOnly property to ensure it can no longer be used, fix failing tests by updating uses to use the hasOnly function) when the time comes to make a "major" release.

(That's not my preferred course of action in principle simply because I'd rather get the root cause corrected now, but backwards compatibility of public interfaces would necessitate it if runner.hasOnly is meant to be public...)

boneskull · 2017-08-07T04:00:05Z

if it's documented or marked @api or @public then yes. otherwise search github and see if anyone is consuming it (unlikely but you never know).

I have no problems with a major release. lets fix root causes.

ScottFreeCode · 2017-08-08T15:01:50Z

Quick assessment on that:

Runner's this.hasOnly is only used in Runner.prototype.run, which is public but does not document hasOnly in the comment; no other reference to hasOnly the property is in lib/runner.js (the only other hasOnly is the function).
runner.hasOnly is set by the Mocha.prototype.run function, which is public but merely copies the property from its this.options, with no mention of this.options in its comment doc.
this.options is set by the Mocha constructor, which is public but has a list of options that hasOnly is not one of.
There are no other mentions of hasOnly in lib/mocha.js either.
lib/interfaces/common.js sets mocha.options.hasOnly in its only functions and (redundantly?) in one of the create functions. None of these are documented in comments as being public -- the whole file seems to be intended for implementing the actual interfaces. (Speaking of which, the proposed change should not have any additional backwards-compatibility issues with properly written custom interfaces, as the interface would currently need to be setting both hasOnly and _onlyTests/_onlySuites, and we'd merely be making the former ignored in favor of the latter.)
No other files besides these three reference hasOnly.

Seems like a bit of a side-channel on top of everything else...

jsf-clabot · 2017-08-09T09:26:31Z

All committers have signed the CLA.

ddneat · 2017-08-09T09:28:35Z

Wow, just failed rebasing that one. Will fix that.

coveralls · 2017-08-09T09:44:36Z

Changes Unknown when pulling 46e82ed on davidspinat:issue-2429 into ** on mochajs:master**.

coveralls · 2017-08-09T10:00:46Z

Coverage increased (+0.04%) to 89.062% when pulling e723bda on davidspinat:issue-2429 into 075bd51 on mochajs:master.

ddneat · 2017-08-09T13:17:46Z

@ScottFreeCode @boneskull

Just updated the PR to include the requested changes. Please let me know if i didn't manage to meet your feedback. I'm happy to help.

Rebase with master
Remove all additional occurrences of the hasOnly property

Cheers.

coveralls · 2017-08-09T13:37:52Z

Coverage increased (+0.02%) to 89.042% when pulling 6486de0 on davidspinat:issue-2429 into 075bd51 on mochajs:master.

ScottFreeCode · 2017-08-11T05:31:17Z

[Editted to add] TL;DR: It doesn't look like this will affect anyone on GitHub if we remove runner.hasOnly but keep options.hasOnly; but even if we remove them both it doesn't look like we're likely to affect more than a couple uses total.

Went through all the GitHub code results (yes, all forty pages) for hasOnly extension:js extension:coffee extension:ts extension:typescript extension:flow extension:es extension:mjs extension:jsx (also looked at extension:cs but it gave me seven pages of C# and no CoffeeScript) and here's what I found:

irrelevant:
- hasOnly as a function, or as a variable rather than as a property (so obviously not it)
- other test runners I'd never heard of
- hasOnly differently capitalized and/or as the start of some other name instead of as the whole word/identifier (so also obviously not it)
- forks of Mocha such as mocha-nightwatch
- copies of the Mocha code in other projects such as https://github.com/Dudinia/Arithmetic/tree/3c90c6e4b62b5a3cdf064b1775a884993a5ead47/TestProject/lib/mocha
relevant:
- this: https://github.com/pflannery/mocha-ui-esm/blob/8a6440f42685bfd6c1f2b5f0576c1c28abeb5c3c/index.js#L27 which seems to be setting the flag and therefore as previously discussed shouldn't be affected?
- this: https://github.com/malexandert/metamocha/blob/c36b077e5c215595ec2ffd8fae57f6032846a071/lib/metadata_ui.js#L88 which I'm somewhat more confident is setting the flag such that it won't be affected as previous discussed
- this: https://github.com/twolfson/multi-image-mergetool/blob/43c06fb4d5c8d751ccbd06cb7910817e7197a5b8/test/browser/utils/application.js#L127-L151 (see next)
- ...and this: https://github.com/twolfson/react-playground/blob/5e559f37d3106a340781de5d5d6cfc3d82715f66/test/common/utils/react.js#L35-L61 which seem to be a couple instances of reading (rather than writing) options.hasOnly that would therfore be affected -- BUT, I believe we could remove the runner.hasOnly = options.hasOnly line without removing the setting of options.hasOnly = true and avoid affecting them until a semver "major" release if we want to be cautious.

I'm marking my prior request for changes as having been satisfied, in any case.

ScottFreeCode · 2017-09-29T01:20:22Z

Still trying to figure out if I'm merging master into this correctly (which was necessary because conflicts); there's one failing test, looking into that.

coveralls · 2017-09-29T02:07:06Z

Coverage increased (+0.03%) to 89.164% when pulling 176ca11 on davidspinat:issue-2429 into 81e16c6 on mochajs:master.

ScottFreeCode · 2017-09-29T02:07:28Z

lib/runner.js

@@ -429,7 +429,7 @@ Runner.prototype.runTest = function (fn) {
  if (!test) {
    return;
  }
-  if (this.forbidOnly && this.hasOnly) {
+  if (this.forbidOnly && hasOnly(this.parents().reverse()[0])) {


...Just realized this still isn't right, because it won't work for tests outside of any describe. Remind me to add a test for that, but first, gonna fix this and merge...

coveralls · 2017-09-29T02:17:50Z

Coverage increased (+0.03%) to 89.164% when pulling 2a2cdc5 on davidspinat:issue-2429 into 4cbaec7 on mochajs:master.

(mochajs#2544) * Fix .only() behaviour when running with watch option * Remove all occurrences of the hasOnly property

ddneat mentioned this pull request Oct 17, 2016

--watch option works strangely with .only() #2429

Closed

boneskull added the needs-cla label Nov 1, 2016

boneskull added status: waiting for author waiting on response from OP - more information needed and removed needs-cla labels Nov 24, 2016

glenjamin mentioned this pull request Mar 16, 2017

Expose beforeAll/beforeEach/afterAll/afterEach in require'd files #764

Closed

xavhan approved these changes Mar 31, 2017

View reviewed changes

boneskull reviewed Aug 1, 2017

View reviewed changes

boneskull previously requested changes Aug 1, 2017

View reviewed changes

boneskull requested a review from ScottFreeCode August 1, 2017 23:59

ScottFreeCode suggested changes Aug 6, 2017

View reviewed changes

Fix .only() behaviour when running with watch option

e723bda

ddneat force-pushed the issue-2429 branch from 46e82ed to e723bda Compare August 9, 2017 09:40

Remove all occurrences of the hasOnly property

6486de0

ddneat force-pushed the issue-2429 branch from 048ead3 to 6486de0 Compare August 9, 2017 13:11

ScottFreeCode approved these changes Aug 11, 2017

View reviewed changes

ScottFreeCode force-pushed the issue-2429 branch from 3182c61 to 6486de0 Compare September 29, 2017 01:06

ScottFreeCode force-pushed the issue-2429 branch from 8b81379 to 6486de0 Compare September 29, 2017 01:54

ScottFreeCode reviewed Sep 29, 2017

View reviewed changes

ScottFreeCode force-pushed the issue-2429 branch from 176ca11 to 6486de0 Compare September 29, 2017 02:08

Merge branch 'master' into issue-2429

2a2cdc5

ScottFreeCode merged commit f01f66e into mochajs:master Sep 29, 2017

ScottFreeCode mentioned this pull request Sep 29, 2017

🛠️ Repo: Ensure Mocha's tests are checking forbid with/without suite #3032

Open

ScottFreeCode mentioned this pull request Nov 15, 2017

Mocha strange behavior with adding/remove only statement while using watch flag #3108

Closed

boneskull removed the status: needs review a maintainer should (re-)review this pull request label Dec 12, 2017

sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019

Fix .only() behaviour when running with watch option, fixes mochajs#2429

8fd92d4

(mochajs#2544) * Fix .only() behaviour when running with watch option * Remove all occurrences of the hasOnly property

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix .only() behaviour when running with watch option, fixes #2429 #2544

Fix .only() behaviour when running with watch option, fixes #2429 #2544

ddneat commented Oct 16, 2016

boneskull commented Nov 1, 2016 •

edited

Loading

ddneat commented Nov 2, 2016

ddneat commented Nov 23, 2016

boneskull commented Nov 24, 2016

ddneat commented Jan 2, 2017 •

edited

Loading

jzaefferer commented Jan 11, 2017

ddneat commented Feb 3, 2017

threepears commented Jun 20, 2017 •

edited

Loading

juank11memphis commented Jul 1, 2017

ddneat commented Jul 24, 2017

boneskull commented Aug 1, 2017

boneskull commented Aug 1, 2017

boneskull Aug 1, 2017

boneskull Aug 1, 2017

ScottFreeCode Aug 2, 2017

ScottFreeCode Sep 29, 2017 •

edited

Loading

boneskull left a comment

boneskull Aug 1, 2017

jzaefferer commented Aug 2, 2017

ScottFreeCode left a comment

ScottFreeCode commented Aug 6, 2017

boneskull commented Aug 7, 2017

ScottFreeCode commented Aug 8, 2017

jsf-clabot commented Aug 9, 2017 •

edited

Loading

ddneat commented Aug 9, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

ddneat commented Aug 9, 2017

coveralls commented Aug 9, 2017

ScottFreeCode commented Aug 11, 2017 •

edited

Loading

ScottFreeCode commented Sep 29, 2017

coveralls commented Sep 29, 2017

ScottFreeCode Sep 29, 2017

coveralls commented Sep 29, 2017

Fix .only() behaviour when running with watch option, fixes #2429 #2544

Fix .only() behaviour when running with watch option, fixes #2429 #2544

Conversation

ddneat commented Oct 16, 2016

boneskull commented Nov 1, 2016 • edited Loading

ddneat commented Nov 2, 2016

ddneat commented Nov 23, 2016

boneskull commented Nov 24, 2016

ddneat commented Jan 2, 2017 • edited Loading

jzaefferer commented Jan 11, 2017

ddneat commented Feb 3, 2017

threepears commented Jun 20, 2017 • edited Loading

juank11memphis commented Jul 1, 2017

ddneat commented Jul 24, 2017

boneskull commented Aug 1, 2017

boneskull commented Aug 1, 2017

boneskull Aug 1, 2017

Choose a reason for hiding this comment

boneskull Aug 1, 2017

Choose a reason for hiding this comment

ScottFreeCode Aug 2, 2017

Choose a reason for hiding this comment

ScottFreeCode Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

boneskull left a comment

Choose a reason for hiding this comment

boneskull Aug 1, 2017

Choose a reason for hiding this comment

jzaefferer commented Aug 2, 2017

ScottFreeCode left a comment

Choose a reason for hiding this comment

on design and concerns

on what is required of this PR (including intersection with forbidding)

ScottFreeCode commented Aug 6, 2017

boneskull commented Aug 7, 2017

ScottFreeCode commented Aug 8, 2017

jsf-clabot commented Aug 9, 2017 • edited Loading

ddneat commented Aug 9, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

ddneat commented Aug 9, 2017

coveralls commented Aug 9, 2017

ScottFreeCode commented Aug 11, 2017 • edited Loading

ScottFreeCode commented Sep 29, 2017

coveralls commented Sep 29, 2017

ScottFreeCode Sep 29, 2017

Choose a reason for hiding this comment

coveralls commented Sep 29, 2017

boneskull commented Nov 1, 2016 •

edited

Loading

ddneat commented Jan 2, 2017 •

edited

Loading

threepears commented Jun 20, 2017 •

edited

Loading

ScottFreeCode Sep 29, 2017 •

edited

Loading

jsf-clabot commented Aug 9, 2017 •

edited

Loading

ScottFreeCode commented Aug 11, 2017 •

edited

Loading