Events: Use EventEmitter model for logging system #882

Closed
wants to merge 6 commits into
from

Conversation

8 participants
@leobalter
Member

leobalter commented Oct 22, 2015

Closes #422
Replaces #644

This is a rebased branch from @JamesMGreene's work on #644 to finish the EventEmitter.


[WIP]

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 22, 2015

Member

I still need to figure out a way to run both logs and events tests on node.

Member

leobalter commented Oct 22, 2015

I still need to figure out a way to run both logs and events tests on node.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 25, 2015

Member

I never involved myself too much on the js-reporters or the Event Emitter before.

Now that I'm trying to finish this pull request I have some thoughts and concerns:

From js-reporters perspective, we have one non-compliant event: the assert event. It's a great feature, but it sounds like a missing piece there.

js-reporters is a good project but in my own PoV, it's a great test report adapter to connect many test frameworks to many other test tools, like Karma, Grunt, Browserstack, etc. On the other side, I wouldn't like to use it for actual reporters (like html, console, tap, etc).

This PR does not adapt QUnit with the event details for js-reporters, and they are not consistent yet: js-reporters/js-reporters#30.

It looks like js-reporters does not support ES3 and even PhantomJS (using Function.prototype.bind without a fallback). QUnit still supports ES3 environments. We can't just ship it along with QUnit 1.x without breaking anything else.

For an actual reporter and adapter, we could also solve it easily if we have an tap output. Everything else could parse a tap log, as every test framework should do it just fine as well. Maybe if we have a method to call on a QUnit.done or an 'runEnd' event, we could return a string containing the whole tap log to be used on any adapter. That does not avoid implementing a real time tap reporter.


With that said, my suggestion is to land this PR as it is and work on tap reporter on the top of it (without using js-reporters). The events details should be addressed only after js-reporters/js-reporters#30 is done.

Member

leobalter commented Oct 25, 2015

I never involved myself too much on the js-reporters or the Event Emitter before.

Now that I'm trying to finish this pull request I have some thoughts and concerns:

From js-reporters perspective, we have one non-compliant event: the assert event. It's a great feature, but it sounds like a missing piece there.

js-reporters is a good project but in my own PoV, it's a great test report adapter to connect many test frameworks to many other test tools, like Karma, Grunt, Browserstack, etc. On the other side, I wouldn't like to use it for actual reporters (like html, console, tap, etc).

This PR does not adapt QUnit with the event details for js-reporters, and they are not consistent yet: js-reporters/js-reporters#30.

It looks like js-reporters does not support ES3 and even PhantomJS (using Function.prototype.bind without a fallback). QUnit still supports ES3 environments. We can't just ship it along with QUnit 1.x without breaking anything else.

For an actual reporter and adapter, we could also solve it easily if we have an tap output. Everything else could parse a tap log, as every test framework should do it just fine as well. Maybe if we have a method to call on a QUnit.done or an 'runEnd' event, we could return a string containing the whole tap log to be used on any adapter. That does not avoid implementing a real time tap reporter.


With that said, my suggestion is to land this PR as it is and work on tap reporter on the top of it (without using js-reporters). The events details should be addressed only after js-reporters/js-reporters#30 is done.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 25, 2015

Member

a1f766a solves the problem with running both events and logs on the node tests. (the commit reference will chance within the next rebase/squash).

Member

leobalter commented Oct 25, 2015

a1f766a solves the problem with running both events and logs on the node tests. (the commit reference will chance within the next rebase/squash).

src/core/logging.js
+ throw new Error( "Emitting QUnit events requires an event type" );
+ }
+
+ callbacks = listeners[ type ];

This comment has been minimized.

@Krinkle

Krinkle Oct 26, 2015

Member

May wanna call .slice() here at the end to ensure a consistent event run.

@Krinkle

Krinkle Oct 26, 2015

Member

May wanna call .slice() here at the end to ensure a consistent event run.

src/core/logging.js
+
+ callbacks = listeners[ type ];
+ if ( callbacks ) {
+ for ( i = 0, len = callbacks.length; i < len; i++ ) {

This comment has been minimized.

@Krinkle

Krinkle Oct 26, 2015

Member

I think caching .length has become an anti-pattern now.

@Krinkle

Krinkle Oct 26, 2015

Member

I think caching .length has become an anti-pattern now.

This comment has been minimized.

@leobalter

leobalter Oct 26, 2015

Member

That's interesting, didn't see it as an anti-pattern anywhere yet. I'll change it anyway.

@leobalter

leobalter Oct 26, 2015

Member

That's interesting, didn't see it as an anti-pattern anywhere yet. I'll change it anyway.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 27, 2015

Member

@Krinkle 1c65763 calls slice() and bf2e02c does not cache .length

Member

leobalter commented Oct 27, 2015

@Krinkle 1c65763 calls slice() and bf2e02c does not cache .length

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 27, 2015

Member

@leobalter I appreciate that you spent some time looking into js-reporters! This looks like a great opportunity to improve the js-reporters spec, instead of partially ignoring it.

Since there a quite a few details to discuss, could we do a Skype/Hangouts call sometime this week? Hopefully @fcarstens can join as well.

Member

jzaefferer commented Oct 27, 2015

@leobalter I appreciate that you spent some time looking into js-reporters! This looks like a great opportunity to improve the js-reporters spec, instead of partially ignoring it.

Since there a quite a few details to discuss, could we do a Skype/Hangouts call sometime this week? Hopefully @fcarstens can join as well.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 27, 2015

Member

Since there a quite a few details to discuss, could we do a Skype/Hangouts call sometime this week?

I'm in!

Member

leobalter commented Oct 27, 2015

Since there a quite a few details to discuss, could we do a Skype/Hangouts call sometime this week?

I'm in!

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Dec 9, 2015

Member

Can we separate the addition of EventEmitter from the test-on-node refactoring? Either land that first, or keep it out for now. I'd like to review and land this pull request this month.

Also, I'd like to land it without changing existing code or tests that use events. Only add a few tests for the EventEmitter.

In a later patch, when we deprecate and introduce warnings for the old code, we can update the existing tests. That way I can have greater confidence in it not breaking any existing code and keeps the patch more minimal.

Member

Krinkle commented Dec 9, 2015

Can we separate the addition of EventEmitter from the test-on-node refactoring? Either land that first, or keep it out for now. I'd like to review and land this pull request this month.

Also, I'd like to land it without changing existing code or tests that use events. Only add a few tests for the EventEmitter.

In a later patch, when we deprecate and introduce warnings for the old code, we can update the existing tests. That way I can have greater confidence in it not breaking any existing code and keeps the patch more minimal.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 9, 2015

Member

Sure. I still need to work on the event details for this PR as well.

Member

leobalter commented Dec 9, 2015

Sure. I still need to work on the event details for this PR as well.

@Krinkle Krinkle changed the title from Events: QUnit logging system uses EventEmitter to Events: Use EventEmitter model for logging system Dec 9, 2015

@leobalter leobalter referenced this pull request Jan 18, 2016

Closed

Core: Introduce before/after hooks for modules #919

11 of 11 tasks complete
@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Feb 1, 2016

Member

This is rebased and the test-on-node refactoring got its own PR (more rebases on the way).

I still need to finish the work on the events, which will consume some time but it's better to have new events with legacy details.

Member

leobalter commented Feb 1, 2016

This is rebased and the test-on-node refactoring got its own PR (more rebases on the way).

I still need to finish the work on the events, which will consume some time but it's better to have new events with legacy details.

"moduleStart", "moduleDone" ];
+ var dictionary = {

This comment has been minimized.

@jzaefferer jzaefferer referenced this pull request in js-reporters/js-reporters Mar 2, 2016

Closed

Help implement js-reporters in QUnit #32

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 18, 2016

Member

Since #981 was closed, bringing this up here again:

Since it looks like the new event emitter API won't make it into 2.0.0, can we release the new API somewhere in a 2.x release, while keeping the existing callbacks intact? We could then remove the old callbacks in 3.0.

New events with old event data (or both old + new data) seems like a bad idea, so I think it would be better to add the new API with new data, then eventually remove the old API....

Member

jzaefferer commented Apr 18, 2016

Since #981 was closed, bringing this up here again:

Since it looks like the new event emitter API won't make it into 2.0.0, can we release the new API somewhere in a 2.x release, while keeping the existing callbacks intact? We could then remove the old callbacks in 3.0.

New events with old event data (or both old + new data) seems like a bad idea, so I think it would be better to add the new API with new data, then eventually remove the old API....

@flore77

This comment has been minimized.

Show comment
Hide comment
@flore77

flore77 Jul 21, 2016

Hey,

I would like to continue this and to integrate the js-reporters standard into QUnit. How should I start? Should I rebase this onto master ? Thanks.

flore77 commented Jul 21, 2016

Hey,

I would like to continue this and to integrate the js-reporters standard into QUnit. How should I start? Should I rebase this onto master ? Thanks.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jul 25, 2016

Member

@trentmwillis since you're working on the conversion to ES6 modules, rebasing this now probably makes no sense, right? Any idea how long you'll be working on that?

Member

jzaefferer commented Jul 25, 2016

@trentmwillis since you're working on the conversion to ES6 modules, rebasing this now probably makes no sense, right? Any idea how long you'll be working on that?

@trentmwillis

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Jul 25, 2016

Member

@jzaefferer I'm hoping to have a PR ready within the next couple of days (mainly restructuring/organizing things right now). I don't necessarily think it'd be a bad idea to rebase this as the ES6 modules changes will be minimally invasive into the actual logic (mainly just adding import/export statements).

Member

trentmwillis commented Jul 25, 2016

@jzaefferer I'm hoping to have a PR ready within the next couple of days (mainly restructuring/organizing things right now). I don't necessarily think it'd be a bad idea to rebase this as the ES6 modules changes will be minimally invasive into the actual logic (mainly just adding import/export statements).

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jul 25, 2016

Member

Gotcha, thanks.

@flore77 Let's try an initial rebase soon. If its non-trivial to resolve, we can still discuss when and how to make it work.

Member

jzaefferer commented Jul 25, 2016

Gotcha, thanks.

@flore77 Let's try an initial rebase soon. If its non-trivial to resolve, we can still discuss when and how to make it work.

@trentmwillis

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Aug 6, 2016

Member

Closing this in favor of #1026.

Member

trentmwillis commented Aug 6, 2016

Closing this in favor of #1026.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment