afterEach inside beforeEach and mocha timing issues.

[tomhicks]

Just spent ages figuring out why our tests seem to slow down about half way through a test suite and continue slowly from then on.

Basically, through what looks like a git merge, an afterEach call had got inside a beforeEach call.

This seemingly causes the afterEach to be run in the outer context (or whatever mocha calls the describe-scoped before/after mechanism) which was adding a load of afterEach calls for every subsequent it in the suite, causing every test thereafter to run slower.

Mocha's slow test reporting was not picking this up and it was a nightmare to figure out.

This brings up two issues:

1. Should the mocha "slow" timing take into account beforeEach and afterEach calls? It doesn't seem to, and if it did it would have made debugging this a whole lot easier.
2. Should there be some warning/error about running an afterEach inside a beforeEach? I can't think of a reason why you would ever want to do this?

Example:

// test1.js
describe("first suite", function() {
  beforeEach(function() {
    // setup

    afterEach(function() {
      console.log("after each first file")
    })
  })

  it("first test", function () { console.log("test1") })

  it("second test", function () { console.log("test2") })

  it("third test", function() { console.log("test3") })
})

// test2.js

describe("second test", function() {
  it("fourth test", function() { console.log("test4") })
})

// output
test1
after each first file
test2
after each first file
after each first file
test 3
after each first file
after each first file
after each first file
test 4
after each first file
after each first file
after each first file

As you can see, this adds an extra after step each time an it is run. This resulted in ~10,000 extra calls being made to the afterEach in one test file, which slowed our tests down significantly. These tests were definitely taking more than the --slow timeout, but they were not being reported as slow.

Expected output would be an error being thrown that afterEach cannot be run inside beforeEach.

Obviously, this is a mistake in the test code, but an error would have been handy. This would also be avoided by using a proper language that doesn't have significant whitespace and linting the indentation, but I feel like mocha could have been more helpful here.

[rodoabad]

I'd love this check to be added to mocha but I feel like an ESLint rule could also solve this one real quick.

[ORESoftware]

sorry about that problem that you experienced, frankly Git is not the best with JavaScript..one thing I always do to minimize problems is run a "git merge -Xignore_all_space" like so http://stackoverflow.com/questions/9776527/merging-without-whitespace-conflicts

the problem you experienced is because of the usage of globals

and contrary to what @rodoabad said I don't think ESLint is the right way to prevent the problem you experienced from happening to other people

IMO, what should happen in the future, and what is currently underway, is that without globals, you won't be allowed to call beforeEach inside an afterEach, and an error will be thrown "afterEach/beforeEach is not a function"

so, soon enough, it will look like:

test.beforeEach(function(b){

    b.afterEach(function(){   //will throw an error because b doesn't have the afterEach function as a property

      });
});

just another reason why globals are bad

[kevinburke]

Should the mocha "slow" timing take into account beforeEach and afterEach calls? It doesn't seem to, and if it did it would have made debugging this a whole lot easier.

I just came here to ask this as well, I think it should

[kevinburke]

I think this patch should cover it, if you're interested I can submit Shyp@d8e28a3

[drazisil]

#2244 was started on this, but not completed, if someone else would like to pick it up.

[ScottFreeCode]

I think the primary fix we need here is for all Mocha's interface functions to be treated as errors if called after Mocha starts running tests -- besides this particular unwanted behavior not being detected by timeout, we've also had a few issues where people tried to add tests during a hook on purpose and were surprised it didn't work the way they expected, which as far as I'm aware comes down to the fact that it's not meant to be supported (Mocha operates in two phases: collect tests+hooks, then run them; trying to add more in the running phase breaks too many assumptions in that design).

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!