Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Implement QUnit callbacks event listener style #422

Open
JamesMGreene opened this Issue · 16 comments

3 participants

@JamesMGreene
Collaborator

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
Collaborator

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
Collaborator

@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
Collaborator

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
Collaborator

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
Owner

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
Collaborator

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
Collaborator

@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
Owner

@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
Collaborator

@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
Collaborator

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.

@Krinkle Krinkle closed this
@Krinkle Krinkle reopened this
@JamesMGreene
Collaborator

@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
Owner

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
Collaborator

Sure, works for me.

@JamesMGreene
Collaborator

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

@Krinkle Krinkle was assigned
@Krinkle
Collaborator

I'd like to take this one.

@JamesMGreene
Collaborator

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);
});
@Krinkle Krinkle was unassigned by JamesMGreene
@JamesMGreene JamesMGreene self-assigned this
@JamesMGreene JamesMGreene was unassigned by Krinkle
@JamesMGreene JamesMGreene self-assigned this
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
8bc9fc4
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
7068a99
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
0b49bf5
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
15ec383
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
53022ca
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
ff4495b
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Removed QUnit.off, hid QUnit.emit
Removed QUnit.off completely.
Hid QUnit.emit from the public API.
Renamed all of the events to match @jzaefferer's suggested event names
from js-reporters/js-reporters#1.

Ref #422
77822d3
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added test for duplicate listeners
Ref #422
8c6aaeb
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added deprecation warning comments
Ref #422
0806d87
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
62f338a
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Removed QUnit.off, hid QUnit.emit
Removed QUnit.off completely.
Hid QUnit.emit from the public API.
Renamed all of the events to match @jzaefferer's suggested event names
from js-reporters/js-reporters#1.

Ref #422
9fe8abc
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added test for duplicate listeners
Ref #422
54b78d7
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added deprecation warning comments
Ref #422
e5c5393
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
cdac2de
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added deprecation warning comments
Ref #422
cfd8b53
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
73def02
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Removed QUnit.off, hid QUnit.emit
Removed QUnit.off completely.
Hid QUnit.emit from the public API.
Renamed all of the events to match @jzaefferer's suggested event names
from js-reporters/js-reporters#1.

Ref #422
151200e
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added test for duplicate listeners
Ref #422
006be9f
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added deprecation warning comments
Ref #422
55c04f4
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
54f281c
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: Added deprecation warning comments
Ref #422
2f2dc69
@JamesMGreene JamesMGreene referenced this issue from a commit in JamesMGreene/qunit
@JamesMGreene JamesMGreene Events: QUnit logging system uses EventEmitter
Fixes #422
Closes #644
32dbb9e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.