Allow multiple before/beforeEach/after/afterEach

[gibson042]

It's inconsistent that every callback passed to QUnit.test is preserved, but beforeEach and afterEach overwrite preexisting values: https://jsfiddle.net/k9af94x7/

We should let the module hooks queue just like the tests do.

EDIT: …including before and after.

[trentmwillis]

Since test is a function of the QUnit global while beforeEach / afterEach are functions of the local hooks object I, personally, don't find it that inconsistent, but acknowledge how similar signatures could lead one to believe they are.

That said, is there a use case for needing the hooks to queue? Given that there is no equivalent queuing for non-callback modules, I am hesitant to want to introduce it here. Could we potentially rename the hook functions to be setBeforeEach and setAfterEach to clear up the inconsistency?

[gibson042]

Since test is a function of the QUnit global while beforeEach / afterEach are functions of the local hooks object I, personally, don't find it that inconsistent, but acknowledge how similar signatures could lead one to believe they are.

Exactly. Further, the global-context test is likely on its way out, which will further emphasize the similarity.

That said, is there a use case for needing the hooks to queue? Given that there is no equivalent queuing for non-callback modules, I am hesitant to want to introduce it here. Could we potentially rename the hook functions to be setBeforeEach and setAfterEach to clear up the inconsistency?

I can't think of a pressing case, but module nesting already allows for use of multiple hooks. And I'm not too worried about non-callback modules because a) the limitation is inherent to the { [name]: fn } structure, and b) I'm trying to phase them out. It's true that this issue is motivated entirely by a desire for consistency, but that's not necessarily a bad thing (I know, I know... cue reference).

[leobalter]

I am not a good supporter of nested tests, different than nested modules.

Said that, I like how our api design works with es6 modules bindings and
destructuring bindings:

import { module, test } from './qunitjs';

module( 'foo', { beforeEach } => {
  ...
});

(Typed on my phone, sorry if the code shows up as a mess)

This is our current api, and I like to explore it on this way.

If this convo ends up on ditching out the hooks object argument from
module, we could use the return of module calls to set the hooks in a
chainable way:

QUnit.module( 'foo' )
  .beforeEach( _ => ... )
  .afterEach( _ => ... )
  .before( ... );

This looks great for modules defined without a callbackfn, but might be
confusing with it.

[gibson042]

I like both of your code examples, but they don't really speak to this issue. My point is that beforeEach and afterEach appear analogous to test, but the analogy breaks down with multiple invocation (the former replace, the latter appends). So we should either rename them to setters, or make them append like test does.

I'm in favor of the behavior generalization because it's backwards compatible (and therefore avoids deprecation/removal) and increases consistency—both internally (since callback combination is already possible with module nesting) and externally (since we'd be matching Intern, Jasmine, and Mocha).

[platinumazure]

I like the idea. Any chance this can make the near- to mid-term roadmap?

[platinumazure]

Question: afterEach hooks would be executed in reverse queue order, right?

[gibson042]

An excellent question. Intern guarantees FIFO queue-order, Jasmine guarantees LIFO stack-order, and Mocha uses FIFO queue-order but I couldn't find any assertions protecting that. And of the three, Jasmine seems to be the most intentional—they switched to match Mocha's queue-order in 2.4.0, and reverted back to stack-order in 2.4.1 on the very same day when queue-order broke real-world test suites.

Given the above, I agree that LIFO stack-order is best.

[platinumazure]

👍 from me on LIFO, that's my preference as well. Thanks!

[platinumazure]

Oh, I probably should have clarified, I'm talking about hooks defined at the same level of nesting. My assumption is that all afterEach hooks in the child module are executed before any in the parent module, regardless of whether we use FIFO or LIFO order for hooks declared in the same module. Please let me know if this is not or should not be the case.

[gibson042]

Thanks for the explicit statement. I was assuming the same.

[JamesMGreene]

Personally, I prefer FIFO within a given level (like Mocha's). Feels unexpected to execute them in a different order than what the consumers provided.

From a nesting point of view, of course, they must execute from innermost to outermost.

[platinumazure]

Well, we could have it both ways with a configuration option, since users will have their preferences. @JamesMGreene Thoughts?

[jzaefferer]

I don't have a preference for FIFO or FILO, but please don't make it configurable.

[platinumazure]

@jzaefferer Why no config option?

Folks: Is this perhaps a sign that this is the wrong way to go? I mean, we do have nested modules to allow for multiple levels of before/after/beforeEach/afterEach. We could conceivably fix the developer experience issue (that is, inconsistency of beforeEach(func) being reset vs append, compared to QUnit.module/QUnit.test) by supporting hooks.beforeEach = func and then removing hooks.beforeEach(func) in the next major, although it would stink to have that change after we've already changed the experience when adding nested modules.

I'm just trying to make sure we have a good discussion, that's all. I'm not really attached one way or the other.

[JamesMGreene]

I believe they should definitely remain functions but that they should append. That is the behavior that I would've expected as a consumer as well.

From much personal experience using Mocha, I can testify that having the ability to add multiple beforeEach/afterEach/before/after hooks at a given level is very valuable if you ever need to do a handful async operations without adding in additional control flow logic to manage the state transitions. It is convenient to have and it keeps your test code much cleaner than needing to wrap things in Promises, or use a flow library like "async".

[JamesMGreene]

FWIW as a no-longer-contributor, I don't really see the harm in having the ordering be configurable but it does seem unnecessary as long as it is well documented.

Personally, when in doubt, I would aim to follow what the majority of consumers would naturally expect. For instance, I personally would not expect these hooks to be reordered.

Also, as Jasmine appears to be the only JS testing library to do so — and they attempted to change it to FIFO themselves — I definitely do not think that they are the ones to model this feature after.

[jzaefferer]

I definitely do not think that they are the ones to model this feature after.

It looks like they had to revert because the broke existing testsuites, not because it was a bad choice in the first place.

I agree with James that FIFO makes sense.

As for config: From a high level perspective, this is such a teeny tiny detail that I have a hard time imaging why anyone would want to configure this. It should work out of the box and be documented.

[gibson042]

Jasmine is the only framework providing real-world justification for an explicit decision here, and that decision was LIFO stack-order, expressed most succinctly in jasmine/jasmine#908 (comment) :

Running [afterEach] in the reverse declaration order allows libraries to have global setup and teardown that can be guaranteed to run in the same context.

Jasmine won't be changing the ordering of afterEach for the foreseeable future.

e.g.,

QUnit.module("needs mocks", function( hooks ) {
    // utilizes beforeEach for setup and afterEach for teardown
    configureMocks(hooks);

    // user-specified afterEach; assumes mock data is still accessible
    hooks.afterEach(sanityCheck);

    // tests running against mocks
    QUnit.test("A", …);
    QUnit.test("B", …);
    …
});

Regardless, a big +1 to this:

From a high level perspective, this is such a teeny tiny detail that I have a hard time imaging why anyone would want to configure this. It should work out of the box and be documented.

[platinumazure]

I hadn't considered multiple async hooks, and so @JamesMGreene's points are well taken.

LIFO makes sense to me simply because it allows you to organize nested resources more effectively (IMO). It matches with things like base/derived class construction/destruction and other things where FIFO makes sense before, and LIFO makes sense after.

let myFixture;

QUnit.module("a module", function (hooks) {
    // Setup/teardown root myFixture
    hooks.beforeEach(function (assert) {
        const done = assert.async();
        someAsyncOperation(function (result) {
            myFixture = result;
            done();
        });
    });

    hooks.afterEach(function () {
        dispose(myFixture);
    });

    // Setup/teardown myFixture.someSubFixture
    hooks.beforeEach(function (assert) {
        const done = assert.async();
        anotherAsyncOperation(function (result) {
            myFixture.someSubFixture = result;
            done();
        });
    });

    hooks.afterEach(function () {
         dispose(myFixture.someSubFixture);
    });
});

With FIFO afterEach, though, all I would need to do is group my beforeEach and afterEach functions together and reverse my afterEach functions. It's not the end of the world-- I just wanted to provide an example of why some users might reasonably expect LIFO afterEach.

[gibson042]

With FIFO afterEach, though, all I would need to do is group my beforeEach and afterEach functions together and reverse my afterEach functions.

Agreed, but that assumes such grouping is possible. In the case of third-party libraries setting up and tearing down (sub)fixtures (e.g., one or both of your example setup/teardown pairs not being user code), LIFO makes much more sense because it allows them to operate with one hook rather than two.

[platinumazure]

As for config: From a high level perspective, this is such a teeny tiny detail that I have a hard time imaging why anyone would want to configure this. It should work out of the box and be documented.

I'm certainly okay with just picking the most sensible approach and adding a configuration option later if users demand it.

[platinumazure]

@gibson042 Can you offer a concrete example? (Fair warning, I'm about to jump to Team FIFO, so this might be your last chance to convince anyone to consider LIFO.)

[gibson042]

• ngMock setup/teardown registration and sharedInjector
• Coding With Spike: Building Test Fixtures to Clean up Mocha / Jasmine Tests
• pytest fixtures
• ScalaTest stackable trait pattern

And honestly, it seems like half the justification of beforeAll/afterAll is just exposing a way to accomplish the above with less contention.

[rwjblue]

I was looking to implement a newer interface for ember-qunit (see emberjs/ember-qunit#258), and ran into this issue.

It isn't immediately obvious from the conversation above if we have settled on the ordering, have y'all made any progress on that front?

[trentmwillis]

In my opinion, I don't think it matters a whole lot, but whatever order is chosen needs to be explicitly documented and tested.

I lean towards LIFO ordering because of the third-party lib ordering mentioned above. It is also similar to how multiple callbacks currently work when in nested modules:

Additionally, any hook callbacks on a parent module will wrap the hooks on a nested module. In other words, before and beforeEach callbacks will form a queue while the afterEach and after callbacks will form a stack.

[trentmwillis]

Support for this has been implemented and will be in the next release.