--forbid-only does not error on suite marked only with no tests

[ScottFreeCode]

Both before and after #2874, the --forbid functions are implemented in terms of tests, whereas only is quite aware of suites including empty ones; thus, the following file:

describe.only("nothing", function() {})
it("should fail", function() { throw new Error("Ouch!") })

...passes with 0 tests run instead of failing due to only being forbidden (or ignoring the empty onlyed suite and failing the other test).

[plroebuck]

Don't think it should be an error to have describe.only if it has no tests. In my mind, it's just a shortcut for not having to specify .only for each member test. Having the --forbid functions implemented strictly in terms of tests seems completely reasonable to me.

In the code provided, think it should ignore the empty describe. No idea why the root-level test would not be run though... that seems to be the real bug to be fixed.

[plroebuck]

Given file "empty-only-suite-with-root-test.spec.js"

describe.only("nothing", function() {})
it("should fail", function() { throw new Error("Ouch!") })

Running Mocha as such gives unexpected (incorrect) results:

$ mocha --forbid-only empty-only-suite-with-root-test.spec.js

  0 passing (0ms)

[plroebuck]

Given file "empty-skip-suite-with-root-test.spec.js"

describe.skip("nothing", function() {});
it("should fail", function() { throw new Error("Ouch!") });

Running Mocha as such gives expected results:

$ mocha --forbid-pending empty-skip-suite-with-root-test.spec.js

  1) should fail

  0 passing (4ms)
  1 failing

  1) should fail:
     Error: Ouch!
      at Context.<anonymous> (empty-pending-suite-with-root-test.spec.js:2:38)
      at callFn (/private/var/tmp/mocha-new/lib/runnable.js:392:21)
      at Test.Runnable.run (/private/var/tmp/mocha-new/lib/runnable.js:384:7)
      at Runner.runTest (/private/var/tmp/mocha-new/lib/runner.js:449:10)
      at /private/var/tmp/mocha-new/lib/runner.js:567:12
      at next (/private/var/tmp/mocha-new/lib/runner.js:363:14)
      at /private/var/tmp/mocha-new/lib/runner.js:373:7
      at next (/private/var/tmp/mocha-new/lib/runner.js:297:14)
      at Immediate._onImmediate (/private/var/tmp/mocha-new/lib/runner.js:341:5)

[outsideris]

In the code provided, think it should ignore the empty describe.

Why it should be ignored? --forbid-only's purpose is to avoid adding unintended only. Adding an empty suite with only make tests couldn't fail. In CI, it is not detected.

And in your code provided, --forbid-pending give incorrect result for me.

describe.skip("first", function() {
  it('should pass', function() {});
});

describe('second', function() {
  it("should fail", function() { throw new Error("Ouch!") });
});

With non-empty suite, it throws Error: Pending test forbidden.

mocha --forbid-pending pending.js


  first
    1) should pass

  second
    2) should fail


  0 passing (4ms)
  2 failing

  1) first
       should pass:
     Error: Pending test forbidden
      at next (lib/runner.js:540:25)
      at Runner.runTests (lib/runner.js:614:3)
      at /Users/outsider/Dropbox/projects/github/mocha/lib/runner.js:703:10
      at next (lib/runner.js:297:14)
      at Immediate._onImmediate (lib/runner.js:341:5)

  2) second
       should fail:
     Error: Ouch!
      at Context.<anonymous> (p3.js:6:40)
      at callFn (lib/runnable.js:392:21)
      at Test.Runnable.run (lib/runnable.js:384:7)
      at Runner.runTest (lib/runner.js:449:10)
      at /Users/outsider/Dropbox/projects/github/mocha/lib/runner.js:567:12
      at next (lib/runner.js:363:14)
      at /Users/outsider/Dropbox/projects/github/mocha/lib/runner.js:373:7
      at next (lib/runner.js:297:14)
      at Immediate._onImmediate (lib/runner.js:341:5)

[plroebuck]

In the code provided, think it should ignore the empty describe.

Why it should be ignored? --forbid-only's purpose is to avoid adding unintended only.
Adding an empty suite with only make tests couldn't fail. In CI, it is not detected.

Disagree. More specifically, its purpose is to prevent adding unintended only tests.
As an empty namespace contains none, no foul was committed... describe.only simply provides syntactic sugar for not having to specify it.only for every test within the namespace.

And in your code provided, --forbid-pending give incorrect result for me.

Given file "pending.js":

describe.skip("first", function() {
  it('should pass', function() {});
});

describe('second', function() {
  it("should fail", function() { throw new Error("Ouch!") });
});

It's derivative of my sample specification file, but not mine...

With non-empty suite, it throws Error: Pending test forbidden.

$ mocha --forbid-pending pending.js


  first
    1) should pass

  second
    2) should fail


  0 passing (4ms)
  2 failing

  1) first
       should pass:
     Error: Pending test forbidden
      at next (lib/runner.js:540:25)
      at Runner.runTests (lib/runner.js:614:3)
      at /Users/outsider/Dropbox/projects/github/mocha/lib/runner.js:703:10
      at next (lib/runner.js:297:14)
      at Immediate._onImmediate (lib/runner.js:341:5)

  2) second
       should fail:
     Error: Ouch!
      at Context.<anonymous> (p3.js:6:40)
      at callFn (lib/runnable.js:392:21)
      at Test.Runnable.run (lib/runnable.js:384:7)
      at Runner.runTest (lib/runner.js:449:10)
      at /Users/outsider/Dropbox/projects/github/mocha/lib/runner.js:567:12
      at next (lib/runner.js:363:14)
      at /Users/outsider/Dropbox/projects/github/mocha/lib/runner.js:373:7
      at next (lib/runner.js:297:14)
      at Immediate._onImmediate (lib/runner.js:341:5)

I'd say it's executing exactly as expected. Your first namespace uses describe.skip -- so each test within that namespace should be interpreted as though it had been specified as it.skip. Since you specified --forbid-pending from cmdline, the first test should fail implicitly. The second test throws an error, so it's failure is explicit. So 2 failures should be expected, et voilà!

[outsideris]

More specifically, its purpose is to prevent adding unintended only tests.

I didn't understand it. So, you meant below .only is intended to pass?

describe.only("nothing", function() {})
it("should fail", function() { throw new Error("Ouch!") })

[boneskull]

@plroebuck

Don't think it should be an error to have describe.only if it has no tests. In my mind, it's just a shortcut for not having to specify .only for each member test. Having the --forbid functions implemented strictly in terms of tests seems completely reasonable to me.

I disagree. The point of --forbid-only is to disallow exclusivity. By that, I mean, execution of less than all normally-executed tests.

If it's a describe.only, some tests won't get run. If it's it.only, some tests won't get run. It doesn't matter where it is.

This:

describe.only("nothing", function() {})
it("should fail", function() { throw new Error("Ouch!") })

Should fail using mocha --forbid-only with an error because only is forbidden.

[plroebuck]

Don't think it should be an error to have describe.only if it has no tests. In my mind, it's just a
shortcut for not having to specify .only for each member test. Having the --forbid functions
implemented strictly in terms of tests seems completely reasonable to me.

I disagree. The point of --forbid-only is to disallow exclusivity. By that, I mean, execution of
less than all normally-executed tests.

If it's a describe.only, some tests won't get run. If it's it.only, some tests won't get run. It
doesn't matter where it is.

Not following what you're implying when you say "some tests won't get run". When the describe.only has no tests, nothing would ever be run. describe is just a namespace for grouping after all.

describe.only("nothing", function() {})
it("should fail", function() { throw new Error("Ouch!") })

Should fail using mocha --forbid-only with an error because only is forbidden.

If you're arguing that we should pedantically enforce .only on an empty namespace, fine.

I don't disagree that there's a bug here, just what the bug is.
See no problem with describe.only not failing (as it has no tests); do see a problem that the root level test doesn't run and fail.