Events: Use EventEmitter model for logging system #882

leobalter · 2015-10-22T21:51:31Z

Closes #422
Replaces #644

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

[WIP]

leobalter · 2015-10-22T22:37:43Z

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

leobalter · 2015-10-25T17:20:55Z

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: qunitjs/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 qunitjs/js-reporters#30 is done.

leobalter · 2015-10-25T19:31:57Z

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

Krinkle · 2015-10-26T16:53:42Z

src/core/logging.js

+		throw new Error( "Emitting QUnit events requires an event type" );
+	}
+
+	callbacks = listeners[ type ];


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

leobalter · 2015-10-27T02:58:21Z

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

jzaefferer · 2015-10-27T14:32:13Z

@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 · 2015-10-27T14:57:06Z

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

I'm in!

Krinkle · 2015-12-09T17:49:31Z

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 · 2015-12-09T18:03:46Z

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

Fixes qunitjs#422 Closes qunitjs#644

Every test runs without the noise of other tests, this allows to run multiple loggins tests, like events and logs.

leobalter · 2016-02-01T14:17:03Z

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.

gibson042 · 2016-02-21T19:50:12Z

src/core/logging.js

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


I think this should reference https://github.com/js-reporters/js-reporters#event-names

jzaefferer · 2016-04-18T16:17:39Z

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 · 2016-07-21T20:25:02Z

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 · 2016-07-25T10:40:15Z

@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 · 2016-07-25T15:14:15Z

@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 · 2016-07-25T15:30:38Z

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 · 2016-08-06T01:41:30Z

Closing this in favor of #1026.

leobalter added the in progress label Oct 22, 2015

jquerybot added the CLA: Valid label Oct 22, 2015

leobalter force-pushed the JamesMGreene-EventEmitter branch from 335c878 to 47b7b74 Compare October 22, 2015 22:08

leobalter mentioned this pull request Oct 25, 2015

Events: QUnit logging system uses EventEmitter #644

Closed

leobalter force-pushed the JamesMGreene-EventEmitter branch from 360cd50 to a1f766a Compare October 25, 2015 19:31

Krinkle reviewed Oct 26, 2015
View reviewed changes

leobalter force-pushed the JamesMGreene-EventEmitter branch 2 times, most recently from f8e28ea to 1c65763 Compare October 27, 2015 02:56

This was referenced Oct 30, 2015

QUnit.reporter( reporter [, options ] ); #890

Closed

QUnit.done should provide aggregated results #472

Closed

Meta: The QUnit 2.0 plan #891

Closed

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

leobalter mentioned this pull request Jan 18, 2016

Core: Introduce before/after hooks for modules #919

Closed

11 tasks

leobalter mentioned this pull request Feb 1, 2016

Improve test-on-node grunt task #925

Merged

JamesMGreene and others added 6 commits February 1, 2016 12:12

Core: QUnit logging system uses EventEmitter

46d86fe

Fixes qunitjs#422 Closes qunitjs#644

fixup! Core: QUnit logging system uses EventEmitter

c524ce8

Build: Improve grunt test-on-node task

c6fd2a3

Every test runs without the noise of other tests, this allows to run multiple loggins tests, like events and logs.

Build: Include extra tests on the node tests task

e355596

fixup! Core: QUnit logging system uses EventEmitter

d443282

fixup! Core: QUnit logging system uses EventEmitter

f421ec3

leobalter force-pushed the JamesMGreene-EventEmitter branch from 1c65763 to f421ec3 Compare February 1, 2016 14:13

gibson042 reviewed Feb 21, 2016
View reviewed changes

jzaefferer mentioned this pull request Mar 2, 2016

Help implement js-reporters in QUnit qunitjs/js-reporters#32

Closed

nikhilshagri mentioned this pull request Apr 13, 2016

Events: EventEmitter logging system #981

Closed

leobalter mentioned this pull request Aug 2, 2016

Adopt js reporters standard #1026

Closed

trentmwillis closed this Aug 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Events: Use EventEmitter model for logging system #882

Events: Use EventEmitter model for logging system #882

leobalter commented Oct 22, 2015

leobalter commented Oct 22, 2015

leobalter commented Oct 25, 2015

leobalter commented Oct 25, 2015

Krinkle Oct 26, 2015

leobalter commented Oct 27, 2015

jzaefferer commented Oct 27, 2015

leobalter commented Oct 27, 2015

Krinkle commented Dec 9, 2015

leobalter commented Dec 9, 2015

leobalter commented Feb 1, 2016

gibson042 Feb 21, 2016

jzaefferer commented Apr 18, 2016

flore77 commented Jul 21, 2016

jzaefferer commented Jul 25, 2016

trentmwillis commented Jul 25, 2016

jzaefferer commented Jul 25, 2016

trentmwillis commented Aug 6, 2016

Events: Use EventEmitter model for logging system #882

Events: Use EventEmitter model for logging system #882

Conversation

leobalter commented Oct 22, 2015

leobalter commented Oct 22, 2015

leobalter commented Oct 25, 2015

leobalter commented Oct 25, 2015

Krinkle Oct 26, 2015

Choose a reason for hiding this comment

leobalter commented Oct 27, 2015

jzaefferer commented Oct 27, 2015

leobalter commented Oct 27, 2015

Krinkle commented Dec 9, 2015

leobalter commented Dec 9, 2015

leobalter commented Feb 1, 2016

gibson042 Feb 21, 2016

Choose a reason for hiding this comment

jzaefferer commented Apr 18, 2016

flore77 commented Jul 21, 2016

jzaefferer commented Jul 25, 2016

trentmwillis commented Jul 25, 2016

jzaefferer commented Jul 25, 2016

trentmwillis commented Aug 6, 2016