Core: Run before and after hooks with module context

[raycohen]

The before and after hooks run once per module as long as there is at least one test in the module. Using environment inheritance allows us to use the module context in those hooks, which allows reading the expected changes to the context from a before hook inside nested modules.

This is a breaking change from 2.x behavior. Previously the after hook ran with the last test's context instead of the module context, which allowed it to access things set on this from inside the test. That will no longer be accessible, and some of the tests were relying on that behavior, so I had to refactor them to get that information from variables in scope for both the test and after hook instead.

This PR replaces #1559

Fixes #1328. Ref #869.

[Krinkle]

_(continuing in main-thread, responding to comment about this.testEnvironment = extend({}, module.testEnvironment);)_

@raycohen Yeah, read-only could be a compromise here.

So what we have today:

• test.before() fires moduleStart + testStart event callbacks, which feel "outside" the test execution. Not given assert or this. But, can technically access it, albeit with the now-known strange and surprising behaviours in various cases, does broadly work fine in practice, but definitely not good enough, not up to our standard.
• we perform execution: before hooks, test, after hooks.
• test.finish() fires moduleDone + testEnd event callbacks.

I see a couple of options:

1. Keep clean separation of events and execution. Remove remnant this.testEnvironment.
   • QUnit.config.current.testEnvironment becomes undefined during moduleStart/testStart.
   • We'll later document as footnote in QUnit 3 guide to use QUnit.hooks.before/beforeEach() for this purpose instead.
2. Keep separation. Remove mutation and keep limited read-only compat.
   • give events a fresh throw-away copy of the module env (start) or last test (end), similar to what we do around each test.
   • Between moduleStart+testStart and hooks.before, we will create a new copy of the module env, this time mutable.
   • Then, between hooks.before and hooks.beforeEach/test.run/etc we create another throw-away copy, like today.
3. Interweave events and execution. This would offer clear and clean ordering with regards creating the module env, then copying to run the test, and restoring afterward. No lost writes. No time-travelling.
   • Today:
   • event moduleStart
   • event testStart
   • hook before
   • hook beforeEach
   • test run
   • hook afterEach
   • hook after
   • event testEnd
   • event moduleDone
   • Instead:
   • event moduleStart (testEnv=module env)
   • 🔺 hook before (testEnv=module env)
   • 🔻 event testStart (testEnv=test env)
   • hook beforeEach
   • test run
   • hook afterEach
   • 🔺 event testEnd (testEnv=test env)
   • 🔺 hook after (testEnv=module env)
   • event moduleDone

Summary:

1. Pro: Unchanged order. Intuitive outcome (no surprises/dead-ends). Change is easy to explain. Use hooks for env. Con: Breaks env in events.
2. Pro: Unchanged order. Unchanged support for read-only env in events. Cons: Break env writes in events. Change subtle/difficult to explain (only read-only is permitted). Writes are silently lost (surprising). Content of the env object either continues to be under-specified in edge cases and/or will undergo the same major change as for begin/after hooks. Continue time-travelling between "test" start and "module" hooks.
3. Pro: Unchanged support for event in events (read-write). Intuitive outcome for env lifetime and ordering (no lost writes, no time-travelling.). Neutral: Different execution order, arguably clearer in some ways. Cons: Breaks execution order. Encourages mixing of observing and runtime. Leaves odd entrypoint for writing to module env from current.testEnv, which if we improve that, we might as well go with (1) and recommend use of global hooks.

[raycohen]

Neutral: Different execution order, arguably clearer in some ways.

I would say my initial impression is that it seems odd to include the before and after hooks inside the testStart/testEnd events, so the changed order you have seems clearer or less surprising to me.

It does seem like open 1 is the simplest to explain. Without being aware of any use cases for reading and writing there, I might vote for that. It seems like there's a refactor available that likely gets anyone using it for that a way forward?

[Krinkle]

@raycohen Yeah, same here. By removing env, we remove importance of the other, and when all else is equal I'd rather not change the ordering there to avoid suble changes in external state in people's code. Sounds like option 1 is a good way forward then!

[Krinkle]

I'm slowly comin around around to embracing Object.create() inheritence.

I was pondering if its worth it and possible to avoid this between modules (in the same way we avoid it between module and test. In QUnit 2.x, all context props are owned by the this object, making them visible to Object.keys(this) and filtered for-loops, with consistent behaviour across module hooks, test hooks, and tests.

This patch I believe preserves that for tests, per-test hooks, and non-nested module hooks. But, for nested child modules, the "before" hook would now see a this that has prototypal inheritence to the parent context. I was focussing on how we set up the module hook's env, but completely forgot that we first create env during the module() function, which happens during the registration phase, not during execution. This starts out with props from the options parameter. Copying the parent props to this synchronously, like we do today, is exactly what causes the bug we're trying to solve. It's doing it too early. Inheritence naturally solves that by allowing continous later modification from underneath it.

I briefly considered if we could "park" these options and defer env creation until the test execution phase begins (e.g. at the "before" hook). However that would mean this in the module scope callback lacks parent properties set by the same mechanism, which is a breaking change.

QUnit.module('parent', { foo: 1 }, function () {
    this.foo++;
    QUnit.module('child', { bar: 1 }, function () {
        // sees this.foo=2 and this.bar=1
        var foo = this.foo;
        var bar = this.bar;
        QUnit.test('example', function (assert) {
            assert.equal(foo, 2, 'foo');
            assert.equal(bar, 1, 'bar');
        });
    });
});

I'll cover this with a test, but it shows there's a need to create the env object earlier. The only way to create it correctly and use that same object throughout, is with inheritence. We can't realistically defer env creation, because the callback needs something usable right away. If we really wanted to avoid this behaviour change, we could go deeper and flatten it like today, and also park it for later, and re-create it. But, I think that trades one set of surprises for another.

Given we're doing a semver-major, I'd say let's do this more cleanly as you have it, and document that anything hasOwn-filtering for loops can safely stop doing so. We already guruantee env to be a plain object by copying props. The downside is that convenience functions like Object.keys() can't be used. By comparison, jQuery.each() and jQuery.map() don't filter non-own properties.

I guess the remaining question then is: What level of consistency do we want to go for?

1. Current patch: Both parent and child props are always owned, except during module scope callback, child before hook. and child after hook. The parent props are flattened and owned in beforeEach, test, and afterEach.

   Note that if the property contains a primitive value and is "modified" by child before hook, then that read-write operation (e.g. +=) effectively copies it into the own object, thus unless our assertion checks for separate properties, this is hard to observe. I've fixed this in https://github.com/qunitjs/qunit/pull/1770.

2. Alternative: Always use inheritence for parent module. Each test env is created by using Object.create() from the parent module env, and then extend() to mixin the module's own env. This giving the appearance that within the current module, we always operate on own properties both. Both in before/after hooks at the module level, in test hooks, and in tests. This would mean that Object.keys() will consistently not see properties inherited from parent modules, in before/after hooks (like option 1), and beforeEach/test/afterEach alike.

3. Alternative: Use inheritence for everything, including between current module and current test. Ech test env is created by using Object.create() from the current module env. This would mean that properties created for the current module's tests by before hooks would become inherited properties during test hooks and tests.

4. Alternative: We treat the observable inheritence as a bug. Before the execution begins (at the "before" hook) we flatten the module env and disconnect from live inheritence. In this case, all code consistently sees own properties only, with the exception of the module scope callback. This has the other difference of making late writes from any badly written async parenet module hooks fail / ignored, same as today.

@raycohen What do you think?

Option 3 feels the purest, but also the largest change. Could a use case benefit from this?

I lean towards Option 1 (easy to explain: inheritance exists pre-test, each test clones the current module env; the module env can be seen as a template for the test env), or Option 4 (keep inheritence internal like it is today).

[raycohen]

I think I prefer 1 over 4 because having matching behavior for Object.keys(this) in the module callback and in the before hook seems desirable.

For 3 I like the implementation but I can't think of any usage-based argument for it. Why would user code care which parent module had set up a given env value? If we go with flattening, a user could manually access that info if they needed, e.g

module('parent', function () {
  this.foo = 1;

  module('child', { parentEnv: this }, function () {
    this.hasOwnProperty('foo') // false
    this.parentEnv.hasOwnProperty('foo') // true

    module('deeper child', { parentEnv: this }, function () {
      this.parentEnv.parentEnv.hasOwnProperty('foo') // true
    });
  });
});

We could even (in the future, if we realized a compelling use for it) automatically set up a parentEnv property as a part of each module's own env. So I don't think we lose much by not doing inheritance at the test level as well.

Alternative 2 is interesting but if the goal is behavior the user expects I think Options 1 and 4 are probably better.

So, I'm also leaning towards Option 1