--watch option works strangely with .only()

[fediev]

After adding .only() and removing it, --watch doesn't work correctly. After removing .only from a test case, I expected mocha should test all test cases, but it did not test any case at all.

steps to reproduce

1. mocha test.js --watch with following code.

// test.js 
// Sample code to reproduce 
describe('test with --watch', () => {
  it('should be ok', () => {
    //
  });
  it('should be ok too', () => {
    //
  });
});

// output
  test with --watch
    ✓ should be ok
    ✓ should be ok too

  2 passing (8ms)

2. Add .only to the first test.

// output
  test with --watch
    ✓ should be ok too

  1 passing (2ms)

3. Remove .only from the first test.

// output
  0 passing (1ms)

Expected Behavior

After removing .only, all tests should be tested again.

Environmet

• OS: windows 10 / Ubuntu 16.04.1
• node : 6.3.0
• mocha : 3.0.2

[dobryanskyy]

Looks like is related to #2327

[codepunkt]

Same problem here, glad its already reported.
mocha 3.0.2, node 6.2.1

[boneskull]

Would accept a PR to fix this if it's not too complicated. We don't really have the resources to maintain --watch.

Suggested workaround: use a third-party package to watch files and re-execute mocha

[fediev]

I don't have enough knowledge of mocha internal to fix this problem, but I want to share what I have checked.

I changed bin/_mocha as followed to check runner's state.

// https://github.com/mochajs/mocha/blob/master/bin/_mocha
// line: 355 
  function loadAndRun() {
    try {
      mocha.files = files;
      runAgain = false;
      runner = mocha.run(function(){
        runner = null;
        if (runAgain) {
          rerun();
        }
      });
      // ADDED for testing 
      console.log('in loadAndRun():', runner.total);
    } catch(e) {
      console.log(e.stack);
    }
  }

When I run test with mocha 2.5.3, .only affected runner.total. Without .only, runner.total was 2 and with .only runner.total changed to 1. But, on mocha 3.0.2. .only didn't affect runner.total and it was always 2.

I think runner.total means the total number of test cases, and it should be changed when .only is used, but it is not. So the problem is not from --watch, but from .only's incorrect internal behaviors.

[dobryanskyy]

@boneskull About 3rd party tools you're suggesting. Can you please point me some tool which might work with mocha.opts? Because all examples i see on the internet imply using some task runners like gulp-mocha which do not accept mocha.opts.

[gurdiga]

@dobryanskyy Maybe watch? It has also a handy little CLI tool. 🤓

[codepunkt]

@boneskull you don't have the resources to maintain watch, yet you update the rest of mocha without removing it? i'd rather remove a neglected feature when it's broken than leave it as is.

edit: will see if i find the time to have a look at it later on

[dobryanskyy]

@gurdiga thanks a lot, watch did work for me.

[dobryanskyy]

However, it's a bit slower than using --watch switch from mocha

[bartels]

I've just run into this as well, but I'm using Mocha's api programmatically, so this issue is not limited only to the --watch option.

In my case I've written a little test runner script that uses Mocha's api specifically to work around problems with --watch. The script uses chokidar to watch files and is calling mocha.addFile() and mocha.run() when files change. So far it's working, picking up moved/added/deleted files (--watch doesn't work in these cases). But I've realized this issue with .only is still present.

In looking through the codebase, I noticed interfaces/common.js sets mocha.options.hasOnly = true in a few places. But nowhere in the codebase is it ever set to false. If one calls mocha.run() a second time after removing .only from tests, this option will still be true, and so no tests will be run. I think mocha.options must be global to Mocha itself. Even creating a new instance (with new Mocha()) does not seem to change anything.

A fix here may be to initialize mocha.options.hasOnly to false, perhaps when a new Mocha instance is initialized? I think I've managed to work around this problem in my script by doing new Mocha({hasOnly: false}), and passing it in with mocha options. However, this feels a little hacky since hasOnly seems more like an internal state variable, rather than an option that should be passed into the Mocha constructor.

[bartels]

I suspect that some of the issues experienced with --watch may actually be problems with Mocha breaking in various ways on multiple calls to .run(). Mocha seems to be relying on the process exiting to do its cleanup. Running tests multiple times in the same process (exactly what you'd want to do for good file watch functionality) causes problems. For example, another issue I needed to work around was clearing the require cache in order to get tests to run a second time (see #1938).

I guess the question is whether running tests multiple times in the same process is something that should be supported by the api. Sane file watch functionality is a prime example where this is useful. The suggestion given to replace --watch by re-running the mocha command each time a file changes is not ideal due to it being pretty slow, especially when using something like babel.

[ddneat]

@bartels I also noticed that hasOnly is never updated to false. In my pullrequest #2544 I detach the outdated state of hasOnly and evaluate on each iteration if there is any test or suite with only.

I just wonder if we should fix the hasOnly state of the rootSuite. However we could also try to remove the hasOnly of the rootSuite. What are your opinions?

[bartels]

@davidspinat Your pull request does seem like the best way to fix this. I'm testing it out and seems to work well in my case. Might be good for @boneskull to take a look too. I'm not all that familiar with mocha internals.

Looking at this a bit more, your pull request seems to have removed the need for setting options.hasOnly at all since the check is now handled on each run by looking at the tests & suites themselves. I think the hasOnly option was only being set globally so it would then be passed along to the runner, which your pull request removes the need for, as far as I can tell.

I tried removing all instances of options.hasOnly from lib/interfaces/common.js and lib/mocha.js. All tests are passing, and I haven't noticed any problems. I would say cleaning up the obsolete hasOnly option entirely make sense.

[ScottFreeCode]

Whether that particular PR makes the cut or not, let's make sure we get one source of truth on only-ness. Something similar was done with whether a given test or suite is skipped/pending, if I recall correct.

[ddneat]

@bartels @ScottFreeCode thank you for having a look.

@bartels I agree that it would make most sense to just remove the hasOnly state. When it comes to maintainability holding and updating states like that is just too dangerous as they might get out of sync quite easy.

@ScottFreeCode from my point of view these kind of states are very hard to maintain and should be avoided. However I tried to evaluate the situation and agree, that only-ness should be handled the same way as skipping and pending. Since thats more an architecture discussion, I would rather stick to my proposal #2544 which seems to be a clean fix for the reported bug. Maybe we could have further on a separate discussion about how we could improve holding and updating the suite/test state in mocha.

Should I extend #2544 to also remove all related hasOnly? What are your opinions?

[ScottFreeCode]

I have no strong opinions one way or the other on separating the fix into immediate and underlying/architecture phases, or into "eliminate use of .hasOnly as an extra property" and "remove the now-unused property" phases. Well, I would like them in separate commits at least so we can look at the two steps in the history, but if they're separate commits I'd be fine having them in the same PR. Others might think differently, however; /cc @mochajs/core

That being said, ideally if we could both fix the immediate issue and set up some kind of test for it so we know if the issue comes back (preferably using the programmatic API in a manner similar to --watch as regards this issue, rather than using --watch directly*), that would help ensure that when we do review the architecture we don't break anything with further attempts to improve it. @bartels Do you think you could adapt some of your --watch-replacing code, minus the workaround for the .hasOnly problem (since that should be fixed), to create a test case?

*The more tests we get using the programmatic API, especially for --watch-like behavior, the closer we get to being able to use the programmatic API with third-party watcher tools so --watch isn't needed!

[laggingreflex]

I've also written a tool that uses mocha programmatically and I also faced similar issues. I ended up doing new Mocha() on every file-change event. The only other thing I had to do was to bust require cache of test files myself, but that was something I had to in the project anyways. Would love to hear you guys' opinion on my approach. I wondered why mocha itself doesn't do this.

[colbin8r]

If d22cae8 fixes this, I would love to see this merged!