Cross test pollution through the runnable context

[JamesMaroney]

When building specs, we tend to build up our code under test and attach it to the running context. Sadly, we've noticed that this context is shared across test runs. Here's a minimal failing spec that shows the issue:

var assert= require('assert');
describe('Context Pollution', function(){
    it('First Pass, setting property on context', function(){
      this.pollution = true;
    });

    it('Second Pass, property should not be set', function(){
      assert.equal(undefined, this.pollution);
    });
});

As a stop-gap fix, I added an event handler to the 'test' event which cleans out the test context (keeping your internal keys). I'll put up a pull request with this just in case you like that approach.

[tj]

that's the point of the context, is to persist. You can easily re-define these things in beforeEach hooks if that's the desired effect

[JamesMaroney]

This behavior of the context, though, is strange in that it encourages
coupling between tests.
For instance, if you rely on this behavior, your specs will only continue
to work if they execute in the same order each time.
Also, for any spec which relies on this (intentionally or unintentionally)
can no longer be run in isolation.

I do think that this persistent context should be re-thought a bit, and if
there is a real need for it, then making it just a little harder to get
to.
For instance, I could see instead of using this.myVal instead using
this.persistent.myVal

That way, this possibly polluted space could at least be namespaced and
users would be using it explicitly.

[tj]

I dont use the context at all personal, I just use closures, the context is helpful for the "exports" ui etc though. Plus when would you not be re-defining something in a beforeEach etc? that seems odd, otherwise you wouldn't use the context at all. I dont see why you would use it in a test case like you have there

[JamesMaroney]

We did just use closures in the past, but that also encouraged accidental
pollution within the closure.
Then we moved to only sharing via the context, which ensure that anytime
the setup is done it will be in a beforeEach, it, etc.

Now that we have a spec suite that just broke through 2000 tests, and have
3 development teams adding specs steadily, manually clearing this context
just isn't maintainable.

We are loving mocha, by the way, as it runs through those 2k+ specs in
about 40 seconds!

[tj]

I haven't used it for this purpose personally but I know some people are using it to perform teardowns in after() / afterEach(), for before/after each we could easily new Context per one, vs manually removing like this patch. Personally I still don't really see it as a problem, if you use say this.user in a test-case, when would you ever not initialize that in beforeEach if you're going to use it

[jfirebaugh]

@JamesMaroney is right. Tests should be isolated, and the test runner should make it easy to do the right thing, and let you know when you did something you didn't intend. Consider the following:

describe("Foo", function () {
  context("when initialized with 'bar'", function () {
    beforeEach(function () {
      this.foo = new Foo('bar');
    });

    // lots of specs
  });

  context("when initialized with 'baz'", function () {
    // lots of specs
  });
});

I've forgotten the beforeEach in the second context, and this.foo will still point to the one initialized in the first context. Maybe the specs will produce some false positives, maybe they'll fail in odd ways. If I got error messages that showed that this.foo was undefined, I'd quickly spot the issue.

[jfirebaugh]

Here's my suggestion:

• Change before/after to execute in a shared context (call it c). This would eliminate the surprise of before/after have wrong "this" scope? #253.
• Change each it+each hooks to execute in a fresh, non-shared context created via Object.create(c) (or equivalent shim).

[andreyvit]

I'm +1 on avoiding shared contexts.

@visionmedia, to answer your question: yes, beforeEach hook should be setting things up, but if the setup gets complicated, and you're moving code around between suites, it's easy to forget something and get surprising cross-test dependencies.

As a use case, I love to add some helper funcs to the context along the lines of _.extend(this, ModuleWithHelpers). Those cannot be global because they depend on some state in this. Think of this.project state and this.build(...) helper which uses the current project internally.

Mocha actually has a test for cross-test pollution in test/acceptance/context.js. What we're proposing is to replace this behavior with the opposite one, creating a test-local context for each test (possibly using the global context as its prototype).

Old:

describe('Context', function(){
  beforeEach(function(){
    this.calls = ['before'];
  })

  describe('nested', function(){
    beforeEach(function(){
      this.calls.push('before two');
    })

    it('should work', function(){
      this.calls.should.eql(['before', 'before two']);
      this.calls.push('test');
    })

    after(function(){
      this.calls.should.eql(['before', 'before two', 'test']);
      this.calls.push('after two');
    })
  })

  after(function(){
    this.calls.should.eql(['before', 'before two', 'test', 'after two']);
  })
})

New:

describe('Context', function(){
  beforeEach(function(){
    this.calls = ['before'];
  })

  describe('nested', function(){
    beforeEach(function(){
      this.calls.push('before two');
    })

    it('should work', function(){
      this.calls.should.eql(['before', 'before two']);
      this.calls.push('test');
    })

    afterEach(function(){
      this.calls.should.eql(['before', 'before two', 'test']);
      this.calls.push('after two');
    })

    after(function(){
      (this.calls == null).should.be.true;
    })
  })

  afterEach(function(){
    this.calls.should.eql(['before', 'before two', 'test', 'after two']);
  })

  after(function(){
    (this.calls == null).should.be.true;
  })
})

[park9140]

@andreyvit, @jfirebaugh , and @visionmedia
I believe my pull request #1164 solves this issues in a decently simple manner.

It basically makes each suite's context prototypically inherit from the parent suite's context. This does not guarentee context not to be shared between tests but it does at least limit cross pollution between test suites.

[andreyvit]

@park9140 I think it's a really good idea. Why each suite, though, and not each test? I would expect the context of each test to be separate. Do other people have use cases for a shared context?

[park9140]

@andreyvit the shared test context is maintained because it is a useful pattern to be able to easily share one before setup when you are doing complex bundles of testing against a single setup.

This allows us to do integration style tests against a class that has been set up in a single state without having to waist execution time in setup for each test.

With this fix in place you could easily wrap each set of tests you want to have a shared setup in a new suite describing the setup being done and then execute tests.

At work we try to ensure that absolutely no setup ever happens in a test which helps keep the tests themselves nice and clean, so I never really need individual tests to have a separate context.