Add testTeardown logging callback.

[hjdivad]

This is primarily useful as a way of making global checks that can fail
tests, for example checking that any XMLHttpRequest initiated during a test
finishes before the next test.

@cyril-sf and I have found this useful. What do others think?

[JamesMGreene]

To me, it seems like you should just be using the actual test teardown callback for this. If you're doing certain things across all modules' teardowns, you could certainly just use a function composition pattern when creating your teardown.

Moreover, I also don't believe that you'll be able to assert anything inside of logging callbacks once @Krinkle's Assertion refactor lands.

[hjdivad]

@JamesMGreene thanks for taking the time to look and the quick response. ^_^

So, we do have a common check across many modules. The rule we want to assert is "no test leaves XHRs unresolved". This seems to me most straightforward to express in terms of before/after hooks for tests.

We could wrap this in a helper that all teardowns have to call, or have the helper generate the teardown via composition, but this requires we remember to do this every time we write a new module. Now every module needs to know about the "don't leave XHRs unresolved" concern, at least indirectly. I'd rather be able to separate those concerns.

This feels to me like exactly the sort of thing test suite hooks are good for. Are they intended to be for nothing more than logging?

[JamesMGreene]

For me personally, they are not necessarily just for logging (though primarily, yes) but they are definitely NOT for assertions or cleanup, both of which I would put under teardown.

Another option would be to duck-punch the module method to consume whatever lifecycle handlers are provided and then wrap them up via function composition.

[JamesMGreene]

@jzaefferer @Krinkle Differing opinions?

[hjdivad]

@JamesMGreene do you see a particular harm in having some set of callbacks that can make assertions? Obviously it could only be for callbacks that happen at the right time in the test lifecycle.

Duck-punching module does nicely solve the separation of concerns issue, although it makes it easier to self-troll if one accidentally calls module pre-duckpunch.

[jzaefferer]

I'd rather add setup and teardown callbacks that apply to the entire testsuite. But then its pretty easy to just share stuff between modules.

@hjdivad could you put together an example that uses module teardown (duck punched or not) so that we can focus more on the actual usecase?

[hjdivad]

@jzaefferer we use this in Oasis.js.

• https://github.com/tildeio/oasis.js/blob/master/test/index.html#L48
• https://github.com/tildeio/oasis.js/blob/master/test/helpers/async.js

[hjdivad]

We have an additional patch to QUnit that we're using on Oasis at the moment, but feel free to ignore it: we're just using it to help us debug some async. issues on SauceLabs.

[JamesMGreene]

Not to tangent too much but:

I'd rather add setup and teardown callbacks that apply to the entire testsuite.

@jzaefferer: That's an interesting note. The most common complaint I get from our developers is that they wish there was an additional module-level setup/teardown that only ran at the start and end of the module rather than for each test. Could be handy to implement that idea.

Available offerings:

1. Per-test setup/teardown, defined at the module level

Potential offerings:

1. Per-suite/page setup/teardown, defined at the suite/page level
2. Per-module setup/teardown, defined at the suite/page level
3. Per-test setup/teardown, defined at the suite/page level
4. Per-module setup/teardown, defined at the module level
5. Per-test setup/teardown, defined at the module level — (already available)
6. Per-test setup/teardown, defined at the test level — OK, I wouldn't actually advocate for this one personally but I thought I'd put the full combinatoric down.

[jzaefferer]

an additional module-level setup/teardown that only ran at the start and end of the module

What's the usecase for that?

Per-test setup/teardown, defined at the suite/page level

As mentioned, I see the use for this one, like the ajax checks that apply everywhere. I'd like to see usecases for the others.

[JamesMGreene]

an additional module-level setup/teardown that only ran at the start and end of the module

What's the usecase for that?

Nothing that can't be done at the test setup/teardown level, usually it's just when trying to avoid doing work that is "safe" (e.g. instantiating some object that won't/can't change state) or slow more frequently than necessary when they aren't worried about affecting the atomic nature of the tests.

Here's an example where I think it would make more sense to add that functionality, though: qunit-composite.js#L114-129

[JamesMGreene]

As mentioned, I see the use for this one, like the ajax checks that apply everywhere. I'd like to see usecases for the others.

So, my thoughts:

(1) Per-suite/page setup/teardown, defined at the suite/page level

I don't think this is worthwhile as they can just execute the "setup" operations before executing the first QUnit operation. Similarly, the "teardown" is likely completely unnecessary since the test run will be finished. The only question would be if they wanted to ensure some assertion was true at the end of their test run, this would be the right way to do it [if I'm correct that there shouldn't be assertions in logging callback handlers] as QUnit does not guarantee test execution order.

(2) Per-module setup/teardown, defined at the suite/page level

This makes sense if (4) makes sense.

(3) Per-test setup/teardown, defined at the suite/page level

We agree this one can add value.

(4) Per-module setup/teardown, defined at the module level

See my previous comments.

(5) Per-test setup/teardown, defined at the module level

Already available.

(6) Per-test setup/teardown, defined at the test level

This is mostly stupid... just add it to your test. The only advantage for a consumer here is that QUnit would deal with the try/catch on their behalf.

[JamesMGreene]

Related plugin:

• https://github.com/bahmutov/qunit-once

Related forum threads:

• http://forum.jquery.com/topic/callback-to-run-before-all-tests-in-a-module
• http://forum.jquery.com/topic/qunit-module-setup-and-teardown
• http://forum.jquery.com/topic/i-implemented-qunit-once-extending-module-lifecycle-with-setuponce-and-teardownonce

[hjdivad]

@JamesMGreene I agree with all your comments above.

[JamesMGreene]

CC: @bahmutov

[mrsalt]

I would like to see this change integrated. We need to run some additional post test checks globally (not at the module level). Our use cases are:

1. checks for memory leaks (event registrations that should have been removed)
2. screen comparison tests (we have some custom stuff to enable this...)

[hjdivad]

@JamesMGreene @jzaefferer what's the status on this? Would the PR be accepted if it covered more cases (such as the ones James mentioned above)? Does it seem, in principle, reasonable in its current state?

I guess I'm trying to get two things answered:

1. Does it look like this feature should exist at all? (I think we're by now all agreed the answer is yes, but perhaps I have misunderstood something).
2. Does this PR need to change in some way before it can be accepted? If so, in what way?

[JamesMGreene]

@hjdivad:

(1) Does it look like this feature should exist at all? (I think we're by now all agreed the answer is yes, but perhaps I have misunderstood something).

Yes.

(2) Does this PR need to change in some way before it can be accepted? If so, in what way?

This PR tries to accomplish a goal by adding new logging events. However, it should instead do so via more granularly-defined lifecycle setup/teardown-style functions than currently exist in QUnit.

[JamesMGreene]

For reference, the equivalent idea from Mocha (visionmedia/mocha@lib/mocha.js#L95-L99):

    exports.setup = context.setup || context.beforeEach;
    exports.suiteSetup = context.suiteSetup || context.before;
    exports.suiteTeardown = context.suiteTeardown || context.after;
    exports.teardown = context.teardown || context.afterEach;

Combined with Mocha's infinitely nest-able suites (a parallel task proposed for QUnit in #543), this results in infinitely granular lifecycle setup/teardown covering Points 1-5 of my earlier comment here (Point 6 is not covered, likely because it's stupid 😄).

[jzaefferer]

I've just reviewed this discussion again and discussed it with @leobalter. Of all the suggested additional callbacks, the only one that's reasonable to us is the globally defined setup/teardown per test, "(3) Per-test setup/teardown, defined at the suite/page level" in James' comment above. Since that is a part of the testsuite and not just a reporter, it should not be implemented as a logging event. I've created #633 to suggest an alternative API. Whatever the result of that discussion, this PR is not going to land, so I'm closing it now.