#1164 breaks before-each blocks' contexts

[moll]

v1.18.2 passes, yet the latest change (#1164) to fix context leak breaks the following:

describe("Outer", function() {
  beforeEach(function() { this.context = this })

  describe("Inner", function() {
    beforeEach(function() { this.context.must.equal(this) })
    it("must not fail", function() {})
  })
})

[park9140]

@moll this should fail, as the context is not the same object. However, any test verifying a property set on any outer context will be available via the prototype chain on nested contexts.

For example, your test could be written as the following and succeed.

describe("Outer", function() {
  var outerContext;
  beforeEach(function() { this.context = outerContext = this })

  describe("Inner", function() {
    beforeEach(function() { this.context.must.equal(outerContext) })
    it("must not fail", function() {})
  })
})

or to better illustrate the prototype chain.

describe("Outer", function() {
  beforeEach(function() { this.context = this })

  describe("Inner", function() {
    beforeEach(function() { this.context.must.equal(this.prototype) })
    it("must not fail", function() {})
    describe("Inner", function() {
      beforeEach(function() { this.context.must.equal(this.prototype.prototype) })
      it("must not fail", function() {})
    })
  })
})

Do you have a specific use case that this does not work for?

[moll]

I assume by this.prototype you actually meant using Object.getPrototypeOf or __proto__, right? Cause a regular inheriting object wouldn't have its own prototype as the prototype property.

Anywho, I don't think the current design choice is the best option for implementing before and after each hooks. Why should inner and outer blocks get inherited objects? What is the practical benefit of it? It interferes with taking a reference of the context in an outer before-each block for use later for whatever purpose.

Each test case run must get a new context anyways. If anything, it should only inherit from the context that went through before-alls. For all practical purposes, before/after-eaches should behave as if they're a flat list.

[park9140]

Yea I meant __proto__ just a bit slow in the mornings...

It would be nice if we could have a full new context per test, however that breaks the tooling for shared behaviors

At least by isolating each suite declaration to have it's own context we isolate data created in the nested suite to just that suite. Before this fix if you would add properties to the this object in tests they would be shared across every test run since all tests shared the same root context object.

The practical reason being that I've seen many problems with mocha where people believe that, each test case run must get a new context.

[travisjeffery]

@moll why would you write that test? that tests how mocha is written rather than a use case of its public apis.

[moll]

Oh, don't get me wrong, each test case must get a new context. It's that a single test case should (or must depending on your tolerance) get one context object that is passed through all beforeEach blocks. Creating new inherited once on each level of describe nesting does nothing good as not all properties on those contexts are primitive values and each test case must reset the context anyhow.

Before blocks (which in Mocha mean beforeAll) must run once and the context must revert to the state it was after their run. Inheriting is the best solution here, just not with beforeEach blocks.

In general, this is a pretty much solved design question in more than few libraries. Ruby's RSpec I think does it precisely how I'm describing. ;)

[park9140]

I think you will find that if you investigate how this actually works is very similar to what you describe.

Each describe generates a separate context onto which you can place properties. This allows before and after in each nested describe to apply properties to a context that is separate from each other.

If you look at the implementation you will find that all beforeEach blocks within the same suite are of a shared context.

describe("Outer", function() {
  describe("Inner", function() {
    beforeEach(function() { this.context = this })
    beforeEach(function() { this.context.must.equal(this) })
    it("must not fail", function() {})
  })
})

Based on the below link, I believe you will find that if you compare this functionality to the rspec examples it does not differ in any substantive manner.
rspec before and after hooks.

I actually built this fix because I got caught expecting nested describes to work how they do in rspec.

[moll]

That's all groovy, it's just that the context of an inner beforeEach should (and I'd argue for must) equal the context from the outer. I can't see any benefit of them not being equivalent, just annoyance.

Are you seeing something I don't? Why isn't the context the same?

[moll]

In code, this should also pass:

describe("Outer", function() {
  describe("Inner", function() {
    beforeEach(function() { this.foo = 42 })
    it("must not fail", function() {})
  })

  afterEach(function() { this.foo.must.equal(42 })
})

I didn't try this now, but based on the implementation description I'm certain it doesn't.

[park9140]

Try that same nested describe in rspec. You will find that something defined on the inner context is only available at that context level and below. Not in any external contexts. This helps prevent pollution of the global context by nested sets.

The implementation as it is allows the propagation of the external context into the child context.

I see what you are saying about the beforeEach and each test needing a further context separation from the before. However, we could implement this as an extension upon the sibling context fix, but in my investigations I found it was reasonably complex and caused the existing tests on contexts to fail. This fix gets us 90% of the way to correct context handling with very small impact.

Sample rspec

require 'spec_helper'

describe "before and after callbacks" do
  before(:all) do
    @outer_state = "set in outer before all"
  end

  example "in outer group" do
    @outer_state.should eq("set in outer before all")
  end

  describe "nested group" do
    before(:all) do
      @inner_state = "set in inner before all"
    end

    example "in nested group" do
      @outer_state.should eq("set in outer before all")
      @inner_state.should eq("set in inner before all")
    end

    after(:all) do
      @inner_state.should eq("set in inner before all")
    end
  end

  after(:all) do
    # p @outer_state.nil?
    @inner_state.should eq("set in inner before all") # this line will throw an error stating that @inner_state is nil
    @outer_state.should eq("set in outer before all")
  end
end

[moll]

Umm, the example above talks about before-alls and I was talking about beforeEaches. I have no quarrels about before-alls. ;)

I've implemented beforeAll and beforeEach hooks in similar projects and a correct implementation that covers what I've talked about is a dozen lines or so. You seem to know Mocha on the inside — is it a mess with too much baggage?

Just create one context at the start of the first pass, run it through beforeAll's, save it and then via inheritance or copying give it to all beforeEach blocks in succession and finally to to the test case. Then reverse everything for after*. Case closed. :)

[travisjeffery]

ok. i haven't seen a good argument why contexts should be the same in nested describes while there has been a good argument made for not being the same (to avoid a high number of properties set).

like this: #1195 (comment) - why should that pass if we're saying our api's contract says that nested describes will be able to read its parent context values and write its own?

it also doesn't make much sense to compare the ruby implementation to our js implementation, i.e. with prototypal inheritance we have a more simple and elegant solution.

[moll]

(to avoid a high number of properties set).

I'm not getting this. Both cases will result in the exact same number of properties set.

I would argue that nesting beforeEaches is merely to not repeat yourself. It follows that beforeEaches should act as if they are right before a test case — after all, they run after beforeAlls of the inner-most nesting.

Fiddling with inheritance on the beforeEach level is a fools errand. Any second you put a non-primitive on the context object, you've blown it. I would also bet most people who use contexts use them precisely to set up common domain models or more complex value objects. And now, any changes to that context in an inner hook may or may not reflect in the afterEach of the outer nesting. Talk about adding complexity. Not good.

It makes most sense to compare APIs regardless of languages. But I grant you that the latest RSpec has gotten to an insanely nutty place, so screw them. :)

[park9140]

@moll, maybe it would help to explain what you mean by rewriting and extending the existing context.js tests to show what you mean.

https://github.com/visionmedia/mocha/blob/master/test/acceptance/context.js

[moll]

@park9140 Hehe, turns out even the tests there show the meaningless of inheriting between beforeEaches — a non-primitive assigned to the context is then mutated deep down and accessed later in the outer scope.

Anyways, the test I'd add for now would be precisely the one I presented at the top of the issue here.

[park9140]

@moll do you have a specific use case where that being true helps?

[moll]

Well, other than that being the right thing to do, the most immediate thing it breaks for me is setting up some API testing helpers that use shared models on the context to make requests and if the context changes from between nesting, it messes up.

Something like the following where api creates a set of functions for requests and signin creates an account. The api will then take the account property off of the context and use that for requests.

beforeEach(Helpers.api)
beforeEach(Helpers.signin)

describe("..", function() {
  it(function() { this.api.get("/foo").statusCode.must.equal(404) })  
})

But most of all consistency. JavaScript's references and mutable object make the current behavior meaningless and inconsistent.

[park9140]

Well I now see why this change is an issue for you. When you se up the api object you store a reference to the context this is the changed in the nested describe block so that the API can't use the inner login setup. You can easily solve this by having your signin helper just call the API object you have created to get the correct context to write the user setting.

I am curious how yo manage this since before my fix you could easily end up with API tests logged in due to the globally shared context when you meant them to be unauthenticated. Perhaps you are just very diligent about doing context tear down manually.

While I agree that there are limitations to the existing behavior it would be far more inconstant to try and implement a complex context management system rather than utilize well known JavaScript patterns.

[moll]

Hey. We must have different meanings for inconsistency, as I would argue the fix you proposed is just that — inconsistent because of the problem with object references and complex because it does more than necessary with inheriting at every step. I'm arguing for less steps and less behavior. Or, as I previously asked, are the internals of Mocha badly done so doing the simpler solution is actually harder?

And, as always, popularity isn't an indicator of quality, simplicity nor correctness. ;)

[park9140]

I highly recommend you go find out for yourself. The code is not that complex. Mocha is all about simplicity and utilizing the standard functionality of javascript to provide functionality.

In order to build something like what you are talking about while maintaining the ability to share state between tests via the context and not break existing functionality in mocha would be highly complex. So complex to maintain and build in fact that I considered just forking mocha and making a new library so I wouldn't have to maintain backwards compatibility.

In order to do so we would have to create two separate levels of context, and then join them. Just fyi this part I have actually implemented and is totally non performant and breaks the library simplicity of mocha.

First, in the before/after calls we would need to set up one context object, this context object in order to keep it isolated between describes, would need to act very much like how the current global context operates with the new fix. If we want to fix the inconsistency issue you are talking about you would need to make a perfect "deep clone" of the context object. However, this basically amounts to magic as far as the javascript object model works. I'll refer you to the underscore conversation on this topic which gives a pretty solid conversation on why this is difficult, an mostly inconsistent in implementation. A similar problem exists in most languages and you will find the same issues in context implementations in many testing frameworks including ruby, java, and c#, that attempt to have beforeAll, and afterAll support in their the framework.

The second part is where your beforeEach, afterEach, and test context is generated. The simple part is creating a new context before we start the test setup, as you have said. The harder part is merging in and managing the previously built shared context. Before we start each individual test we have to individually place each of the properties on the outer context onto our new context. Then because there is the possibility of ending up with changes to those properties that are expected to be reflected in the next test, we copy those properties back onto the the outer context.

To be fair if we just dropped backwards compatibility on the describe context being shared onto the individual test context this would be simpler in that we could just make the outer context availability available as a single property like 'describeContext' on the test context.

While this all may seem simple it adds a significant amount of processing and code compared to the current mocha implementation that is the bread and butter of why this framework is so loved.

The current solution is just a compromise between popular opinion, javascript limitations, and expected functionality.

[moll]

Thanks for the long description. I do however get a feeling you're overcomplicating or overthinking things. I might've already described the solution I propose which would be for each describe to create a new context by inheriting from the parent context. And one extra inheritance level for the actual test case, as always. Anything else would be up to the developer. No copying of any sorts should happen anywhere.

But, yeah, if you feel like the current behavior should stay in Mocha. I'll put creating a better designed test harness to my todo list. :-) I've been thinking of one for a while now, anyways.
That will suit well with my despise of Should.js (see Must.js). ;-)

[moll]

I just got bit by this change. Again. I had some initialization made in an outer scope's beforeEach block. The inner block reverted that initialization and replaced it with another object. Now because of this change, the original object still lived on in the outer context and the uninitialization that happened in the outer block's afterEach referred to the wrong object.

I still insist this change is a bad decision and counter-intuitive.

In code, something like:

describe("Outer", function() {
  beforeEach(function() { this.something = new Something({a: 1}) })
  afterEach(function() { this.something.close() })

  describe("Inner", function() {
    it("must pass", function() {
      this.something.close()
      this.something = new Something({a: 2})
    })
  })
})

[moll]

To be more precise, every it run should definitely have a new context (like I reported way back in another issue), but every step through befores, its and afters must not.

[park9140]

@moll I agree, however, that is a much more invasive change to the api. You should make a pull request and get it reviewed.

[boneskull]

Am I correct that such a PR would break shared behaviors? @travisjeffery already closed this issue, so if that is the case, it may not be worth bothering with.

It appears placing variables in the function scope would be a reasonable workaround, at least to me, but I'd like to understand why this is unreasonable for others.

[moll]

What were shared behaviors again?
As to the "workaround" — lexical scope might not be available to the after function.

Inheriting in this context is just a Bad Decision, period. You'll shadow when setting this.foo, but obviously not when you changing an inner object of this.foo.bar. Shadowing here is a solution looking for a problem while creating a different problem.

@park9140: That, or I could just write a new test harness from scratch. :-P I've already got what I argue to be a better assertion library out there. 😇

[boneskull]

@moll

Shared Behaviors

Can you please show an example where lexical scope doesn't work for after()?


[moll]

Ah. Thanks. I'm exactly arguing that the change I'm referring to in this issue (the one that creates a new inheritance level for each inner scope) breaks "shared behaviors". I've described a proper solution somewhere above in the comments. In short it would be one context for every beforeEach and afterEach block per test. before and after blocks can use inheritance. *eaches not.

Lexical scopes won't work for functions that are defined somewhere else. Lexical scopes are an anti-pattern as well in this context as they're semantically wrong and would interfere with parallel tests. Even if Mocha doesn't support parallel tests yet, some harness will soon surely.

[moll]

Could this issue be looked again please?

I occasionally use the following pattern (https://github.com/moll/node-mitm/blob/7469b18f89c5d5f2016ef7b940323ad9c0ecea2b/test/index_test.js#L126) to reset and replace an object that was set in a beforeEach block. If the before blocks didn't use inheritance like I've proposed, this wouldn't be an issue. Currently, however, it is, as the replacement this.mitm = Mitm() won't visible to the afterEach and it therefore calls this.mitm.disable, again, on the old object.

[jupiter]

I used to use context in the same way as you @moll, and reluctantly changed all my tests to what is generally considered best practice, i.e. creating a var in the describe scope. It did mean that devs need to be careful not to set these values outside a beforeEach block, and to only use the value set in the beforeEach, which is why I hated it to start out with. But now, the pattern is fairly well established, and (hopefully) doesn't get missed by anyone.

[moll]

Thanks for the explanation. I can't say I agree with it being a best practice though. I think it's not a practice worth following regardless of its popularity and I'd rather avoid it to work around what I described here as a bug. Why else does this context feature exists then. :-)

[jupiter]

True, I don't agree with the isolation of contexts, but if you want to change the value set in a higher-up context, this is the only way to do it.

[moll]

but if you want to change the value set in a higher-up context, this is the only way to do it.

That's true. Right now. ^_^ Hence my revival of this thread.

[jupiter]

It seems to me someone found they liked isolated contexts and because it is a philosophical improvement rather than an actual one, it seems like a battle not worth fighting. :)

[moll]

Well, I too think contexts need to be fresh between test cases, but trying to protect contexts from each-blocks is a fool's errand. Any more complex object beyond a primitive won't be shadowed. I might've already described it fully above.

[jupiter]

I agree. I don't see the value in this rather superficial way of isolating contexts.