Implement QUnit callbacks event listener style

[JamesMGreene]

Open for discussion!

I would like to change the QUnit core logging callbacks (e.g. QUnit.done, etc.) to utilize [theoretical] on, off, and emit methods.

Although we can leave the current callback functions around temporarily for deprecation's sake, the new preferred usage would be as follows (using suggested names):

• QUnit.begin(fn)
  • QUnit.on('run.start', fn)
• QUnit.moduleStart(fn)
  • QUnit.on('module.start', fn)
• QUnit.testStart(fn)
  • QUnit.on('test.start', fn)
• QUnit.log(fn)
  • QUnit.on('assert', fn)
• QUnit.testDone(fn)
  • QUnit.on('test.done', fn)
• QUnit.moduleDone(fn)
  • QUnit.on('module.done', fn)
• QUnit.done(fn)
  • QUnit.on('run.done', fn)

This change would bring with it all of the usual benefits of an EventEmitter-style setup:

• on:
  • Listeners can be added — current
  • Listeners for custom events can be added, e.g. for use in QUnit addons — new
• off:
  • Listeners can be removed — new
• emit:
  • Listeners can be manually triggered, e.g. for use in QUnit addons — new

One more important functionality I would prefer to add is the equivalent of a stopPropagation method (or flag, or return value) that would allow a listener to prevent subsequently added listeners from being triggered. The use case here would again be primarily for QUnit addons.

For example, in QUnit Composite, I would like to do something like the following:

(function(QUnit) {

// Custom event for QUnit Composite
QUnit.on('suite.start', function(data) {
    // log data about a SUITE starting rather than confusing it with a test starting
});

// Custom event for QUnit Composite
QUnit.on('suite.done', function(data) {
    // log data about a SUITE ending rather than confusing it with a test ending
});

QUnit.on('test.start', function(data) {
    if (executingCompositeSuite) {
        QUnit.emit('suite.start', data);
        // Prevent all subsequently added 'test.start' listeners from being triggered
        return false;  // example of a "stopPropagation" functionality via return value
    }
});

QUnit.on('test.done', function(data) {
    if (executingCompositeSuite) {
        QUnit.emit('suite.done', data);
        // Prevent all subsequently added 'test.done' listeners from being triggered
        return false;  // example of a "stopPropagation" functionality via return value
    }
});

// For illustration only; this code already exists [in better form] in PR #408
var executingCompositeSuite = false;
QUnit.extend(QUnit, {
    testSuites: function(suites) {
        // When the first `test` in this set of `suites` starts (i.e. in `setup`), set:
        // executingCompositeSuite = true;

        // Run tests
        // ...

        // When the last `test` in this set of `suites` ends (i.e. in `teardown`), set:
        // executingCompositeSuite = false;
    }
});

})(QUnit);

[Krinkle]

I can't find it in an issue, but this is exactly what I've wanted for a while as well. All for it.

By the way, in this case events for 'suite.start' and 'suite.done' should probably be triggered by QUnit by default, this way other addons don't have to "know" about composite's custom events.

[JamesMGreene]

@Krinkle Apparently this issue lost a bunch of my fenced code block at the end. Does your statement still stand?

I generally agree that QUnit Composite should really be a part of QUnit Core but I know @scottgonzalez wanted to keep it as an addon pretty adamantly. Can't recall what @jzaefferer's opinion was....

[Krinkle]

I meant that the event suite.start / suite.done should be in core (and only fire once by default), I'm not referring to the Composite add-on itself. That way other add-ons that aggregate the information that want to support composite, don't have to special-case that.

[JamesMGreene]

Oh, sure, so QUnit core would always fire those once but QUnit Composite would fire it n times? That makes sense to me.

Although... QUnit core firing it when also using QUnit Composite might actually lead to some confusion with the reporters as well. We'll probably just need to implement it to see how that particular bit will shake out.

[jzaefferer]

I'd like to keep the composite separate, too. As for the suggested changes: I think the goal here should be to make these callbacks only a service API for add-on developers or whoever integrates QUnit into something else, like a CI tool. There shouldn't be any reason for regular QUnit users having to know about these. Which should probably be reflected on the API site as well.

Under that assumptions, those changes are fine. Its makes the API a little bit less convenient (doesn't matter as long as the existing methods are still around), while having more flexibility overall.

The "stop propagation" feature makes sense for the composite case, though it would be nice to a have a reference in another event emitter API that has something similar.

As for the event names: Why the dot inbetween? Is there any advantage over just suitestart, testdone?

[Krinkle]

It nicely groups events, I quite like that actually. It makes it much more predictable and intuitive.

If you want to avoid confusion with namespaces (as in jquery) we may want to use dashes instead.

@jzaefferer Why is it less convenient this way? I think having 1 entry point for binding callbacks with an event name, is actually more convenient if anything. It is certainly more maintainable and easier to document.

[JamesMGreene]

@Krinkle already nailed my thoughts on it.

The dots are a preference of mine which allow for easily grouping (and/or wildcarding subscriptions in some other pub-sub frameworks, e.g. OpenAjax Hub); I'd be fine with dashes, too, as having namespacing available (via the jQuery-style dot syntax) might actually be very handy for addons, e.g. QUnit.on('test-start.qunit-composite-addon', fn).

I also personally prefer this approach for its convenience in reducing the API surface area. The only major inconvenience that comes to mind is lack of Intellisense/auto-complete for the event names vs. having event methods. As a PhantomJS collaborator, I can also note that having all of the callback hooks being top-level properties (in their case) actually gets very annoying... to the point that I actually wrote a wrapper around them: BetterWebPage.

I have never encountered the "stop propagation" functionality in any of the common event emitter libraries. The OpenAjax Hub 2.0 has something similar but not quite the same either, see Line 713 on their latest code.

[jzaefferer]

@JamesMGreene QUnit actually had those top-level properties as well, that was the first generation. The current version is already a huge improvement, but obviously we can do better. Disregard my convenience argument.

I'm okay with dashes as separators, though I still don't see why we need those. Also so far I don't see a need for namespacing. Let's not bother with that unless we really need it.

As for the stop-propagation feature: Is that really necessary to implement the composite add-on properly?

Also related to these callbacks: There was discussion somewhere for adding more information to each callback. The idea is that to display summaries, you don't really care when things start, so testdone could provide a summary of assertions, moduledone a summary of tests, suitedone as summary of everything. Especially the assertion aggregation is something most reporters duplicate right now.

[JamesMGreene]

@JamesMGreene QUnit actually had those top-level properties as well, that was the first generation. The current version is already a huge improvement, but obviously we can do better. Disregard my convenience argument.

Oh, interesting bit of history... I did not know that!

I'm okay with dashes as separators, though I still don't see why we need those. Also so far I don't see a need for namespacing. Let's not bother with that unless we really need it.

OK, so dashes it is for now. Good, @Krinkle?

As for the stop-propagation feature: Is that really necessary to implement the composite add-on properly?

Well, there are a number of ways to do this (and probably more than I'll list) but all of them require changing something or another:

1. Allowing me to stop propagation on the testStart/testDone events and instead firing suiteStart/suiteDone equivalents, as previously mentioned in this issue.
2. Allowing me to somehow add another key-value pair into the data object that gets passed to the testStart/testDone events (e.g. isSuite: true).
3. Make the Composite addon a part of QUnit core.

Also related to these callbacks: There was discussion somewhere for adding more information to each callback. The idea is that to display summaries, you don't really care when things start, so testdone could provide a summary of assertions, moduledone a summary of tests, suitedone as summary of everything. Especially the assertion aggregation is something most reporters duplicate right now.

In #393, I mentioned the idea of rolling up the counts for the XML but I didn't mention rolling them up for the logging callback data. I do think doing so would make total sense, though! We also wanted to add duration to all of the *[Dd]one events as mentioned in #395. Finally, you, me, and @scottgonzalez discussed in IRC about how I wanted a hook to add data to events; see (2) in my previous paragraph for why.

[Krinkle]

I'm okay with dashes as separators [..] Also so far I don't see a need for namespacing. Let's not bother with that unless we really need it.

OK, so dashes it is for now. Good, @Krinkle?

Sounds good.

Well, there are a number of ways to do this [but] all of them require changing something

I'm not sure I follow. In #1 you'd rely on composite firing its callbacks "first", this is imho a pattern one should avoid. In #2, for whom is the isSuite data? If it is only for the addon itself, it should store that information privately instead. #3 is another discussion, but whichever way it could be done in core, that way should be possible outside core as well.

[JamesMGreene]

@Krinkle All of these options are to allow for integration with the various reporters.

I added another option (the original) below as (4). Also added a note to (2).

Updated options:

1. Allowing me to stop propagation on the testStart/testDone events and instead firing suiteStart/suiteDone equivalents, as previously mentioned in this issue.
2. Allowing me to somehow add another key-value pair into the data object that gets passed to the testStart/testDone events (e.g. isSuite: true). This requires the reporters to have more intimate knowledge of the Composite addon than I would like, though.
3. Make the Composite addon a part of QUnit core.
4. Set the executingComposite flag on a globally available object (e.g. QUnit) so that the reporters can check its state during testStart and testDone callbacks. This requires the reporters to have more intimate knowledge of the Composite addon than I would like, though.

[jzaefferer]

Can we implement the new callback style first, then look into the specific issue for the composite add-on again? None of the above looks like it needs to block this ticket. Let's deal with it in a separate ticket. By the time we get there, we'll know more anyway.

[JamesMGreene]

Sure, works for me.

[JamesMGreene]

@Krinkle Did you want to give this one a go or should I?

[Krinkle]

I'd like to take this one.

[JamesMGreene]

Another thing this can enable for the plugins is adding "data preparation" events that allow a plugin author to override/extend the data object of an event that is about to fire. e.g.

QUnit.on('test-start-dataprep', function (data) {
  data.isSuite = true;
  return data;  // or: `QUnit.emit('test-start-datachanged', data);
});

[leobalter]

We don't have QUnit.off but it does not seem to be necessary so far. I'm closing this for now and I believe we should investigate this specific method in a separate issue if someone is interested.

QUnit.on and an internal emit method are properly working!