Mocha: --forbid-only doesn't recognize `it.only` when `before` crashes

[medikoo]

Not sure if it's a feature or a bug, but --forbid-only crashes only in case of approaching describe.only but not it.only.

It caught me by a surprise.

Is it supposed to work that way? I assume it should crash same way it crashes for describe.only

[boneskull]

@medikoo It should be working (at least in master) as I can reproduce the failure
by running:

$ bin/mocha test/integration/fixtures/options/forbid-only/only.fixture.js --forbid-only
  forbid only - test marked with only
    1) test2


  0 passing (5ms)
  1 failing

  1) forbid only - test marked with only
       test2:
     Error: `.only` forbidden
      at Runner.runTest (lib/runner.js:515:8)
      at /Users/boneskull/projects/mochajs/mocha/lib/runner.js:647:12
      at next (lib/runner.js:441:14)
      at /Users/boneskull/projects/mochajs/mocha/lib/runner.js:451:7
      at next (lib/runner.js:362:14)
      at Immediate._onImmediate (lib/runner.js:419:5)

The contents of that file:

'use strict';

describe('forbid only - test marked with only', function() {
  it('test1', function() {});
  it.only('test2', function() {});
  it('test3', function() {});
});

[medikoo]

@boneskull I looked closer into it, and narrowed it down.

The issue is, that it becomes ignored if before, crashes, e.g. following is a nice minimal test case that exposes it:

describe('something', () => {
  before(function() {
    this.skip();
  });
  it.only('only test', () => {});
});

describe('something else', () => {
  it('other test', () => {
    throw new Error('Fail');
  });
});

Error: `.only` forbidden is reported, only if this.skip() is commented out, otherwise we have a successful pass.

To me it already looks as a design issue, fact that those errors are reported at test _runtime_, and not at test _definition_ phase (when mocha gathers all describe and it declarations).

Technically I would expect this error to be thrown before any test being run.

[boneskull]

I haven't independently confirmed this but seems plausible enough

[boneskull]

IIRC @outsideris changed it so describe.skip would be detected at this point, but it sounds like consolidating those checks at suite definition time is what we need to do.

[outsideris]

I confirmed this bug.

[rpgeeganage]

@boneskull ,
I like to help on fixing the bug and contribute.

[rpgeeganage]

@outsideris,
Shall I start working on this?

[rpgeeganage]

I have started looking into the bug 😄 😄

[outsideris]

@rpgeeganage Good. :)

[plroebuck]

describe('something', () => {
  before(function() {
    this.skip();
  });
  it.only('only test', () => {});
});

Error: `.only` forbidden is reported, only if this.skip() is commented out, otherwise we have a successful pass.

To me it already looks as a design issue, fact that those errors are reported at test _runtime_, and not at test _definition_ phase (when mocha gathers all describe and it declarations).

Technically I would expect this error to be thrown before any test being run.

Unsure there is a _specified_ priority order in which tests are filtered out...
Relevant comment from #3630...