Adopt js reporters standard #1026

flore77 · 2016-08-02T20:51:25Z

No description provided.

platinumazure · 2016-08-02T21:13:14Z

Is it just me or do you have conflict markers?

leobalter · 2016-08-02T21:16:10Z

is this only reusing old commits from #882?

flore77 · 2016-08-02T21:50:30Z

This wanted to be a pr only my fork, but I made the pr into the upstream...anyway this should have arrived later, so I guess it is ok.

do you have conflict markers?

@platinumazure yes there were, I made the rebase multiple times.

is this only reusing old commits from #882?

@leobalter yes

I will ping back when I will manged to get it properly done, i.e tests will pass, or if I will get stuck.

flore77 · 2016-08-02T22:56:06Z

The rebase is now complete, there is still work to do, but I think it is a good start 😃

jzaefferer · 2016-08-03T14:32:20Z

I still think this needs to wait for #1025 to land, which is pretty close.

flore77 · 2016-08-04T19:40:22Z

Ok.

In the meanwhile what would be the plan, to deprecate the current API, like adding an warning in 2.0, then implement the standard and to make the 3.0 release? Or to implement the API along with the old one and release a 2.x version and then on 3.0 to get rid of the old one?

leobalter · 2016-08-04T19:58:25Z

I'm a +1 for deprecation, but I would wait for a minimal number of reporters to adapt for the new api, including the HTML Reporter (which might be covered here but I'm writing this in the go).

The new API can be released in a 2.x version, we avoid releasing new features in a major version, just features removals.

We should not worry about the warnings message in this PR, let it be done in a follow up step so we can have an easy process for each part.

flore77 · 2016-08-04T21:10:25Z

Sounds good.

jzaefferer · 2016-08-05T10:29:45Z

reporter/html.js

-		addClass( tests, "hidepass" );
-	}
+QUnit.on( "runStart", function( details ) {
+  var i, moduleObj, tests;


Looks like the indentation got messed up. QUnit uses tabs, not spaces.

In other words, needs to convert from spaces back to tabs here.

Yes, indeed. I need to change my editor config to meet QUnit coding style.

trentmwillis · 2016-08-06T01:46:55Z

+1 to what @leobalter said. Once this has landed I'd also like to see us introduce a non-browser reporter with the EventEmitter API (e.g., a TAP reporter).

leobalter · 2016-08-07T13:27:09Z

I'm planning to rebase the tap reporter on top of this.

I'll do a proper review for this PR this week. Meanwhile, can you help us fixing the CLA issue? Maybe you'll just need to sign it and the problem is solved.

flore77 · 2016-08-07T14:00:01Z

Yes, I will sign the CLA. I will also rebase this again since #1025 has now landed into master.

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.

jzaefferer · 2016-08-10T18:07:12Z

build/tasks/test-on-node.js

 			testActive = true;
 		} );
-		QUnit.log( function( details ) {
+		QUnit.on( "assert", function( details ) {


This is kinda interesting. js-reporters doesn't define an assert event. To render individual assertions in progress, its necessary.

I think its fine for QUnit to add another event, but the object should follow the spec for Assertion.

Personally, I don't find it a good idea, because then everyone could add something. make a little change etc.

Also that's why we have introduced the test assertions property on the testEnd event , which is specifically designed for this.

jzaefferer · 2016-08-10T18:11:07Z

This currently only adopts the event names, not the event properties, right? I think I mentioned this before: Going from old events to new events, but with old properties is not a good approach. New events need to have new event properties. Makes backwards-compat harder (need old events with old props + new events with new props), but that seems the only option to me.

jzaefferer · 2016-08-20T00:09:23Z

src/core.js

 import { sourceFromStacktrace } from "./core/stacktrace";

+import {Suite} from "js-reporters/lib/Data";


jzaefferer · 2016-08-20T00:14:11Z

If I'm reading this correctly, this now leaves the existing callback completely alone, and adds the event emitter as its own (sub)system. Is that right? In 3.0 we'd then remove the old system?

flore77 · 2016-08-20T08:23:36Z

this now leaves the existing callback completely alone, and adds the event emitter as its own (sub)system.

@jzaefferer Yes.

I added tests to test only that the events are emitted and the order is correct (it will not always be source order, it depends on how QUnit executes the tests, but this can be treated in another pr).

Also about this tests can you explain me the setTimeout, I have seen this is done also in testing the old logging callbacks. The timeout is also to big, this tests didn't have the chance to run. I am not sure why they are influencing the other tests if I don't put them in a timeout.

To test the emitted data I must run the tests from js-reporters. Any suggestion how to do it ?

I will have another look through the code (to add comments, ... etc.) and then I will require review from the whole team 😃

flore77 · 2016-08-22T17:39:51Z

I have quite finished the adoption of the events and added tests to test that they are emitted and the emitting order is corect.

One problem has arise out of the nested suites testsuite.

Here is how QUnit executes the testsuite and emits on the old callbacks:

module: module1 > module2 starts
test: test2-1 starts
test: test2-1 ends
module: module1 > module2 ends
module: module1 > module2 > module3 starts
test: test3 starts
test: test3 ends
module: module1 > module2 > module3 ends
module: module1 > module2 starts
test: test2-2 starts
test: test2-2 ends
module: module1 > module2 ends
module: module1 starts
test: test1 starts
test: test1 ends
module: module1 ends

@leobalter @trentmwillis @jzaefferer how you can observe from the above description the start and end of module module1 > module2 is emitted twice. Is this considered correct ?

trentmwillis · 2016-08-24T02:28:00Z

Sorry for not keeping up with this.

the start and end of module module1 > module2 is emitted twice. Is this considered correct ?

I don't believe so. At the least, I do not think it should be correct. The module should only start and end once, any nested modules should be a sub-set of that module, which means that their start and end is "within" the start and end of the parent module.

flore77 · 2016-08-24T08:08:04Z

The module should only start and end once

I agree with you.

any nested modules should be a sub-set of that module, which means that their start and end is "within" the start and end of the parent module

I think this is due to QUnit's flat style and that nested modules were introduced later. The js-reporters standard is how you have described it and I think I have achieved this. So I don't think it is necessary to do it also for the old callbacks, since they will be removed.

flore77 · 2016-09-02T20:18:50Z

The implementation is almost finished, I think there should be only a little bit of work on the emitted data.

To test the emitted data it would be really good to run the tests from the js-reporters repo. But, I am not sure how to do this. What I have thought about is to pull out the tests from js-reporters and to build something like a cli-tool that would test any framework.

To achieve this, the tool would need 2 external things:

a test fixture, which would mimic exactly this ones: https://github.com/js-reporters/js-reporters/tree/master/test/fixtures
a way to run the framework against the test fixture, like this: https://github.com/js-reporters/js-reporters/blob/master/test/integration/adapters-run.js

I found that from this solution all testing frameworks can benefit, also futures ones, with only a little effort they can test their reporting and know that they are compliant to the spec.

What do you think about this?

This is only an idea, so I am open to any other solution.

gibson042 · 2016-09-07T01:48:02Z

I'm in favor of pursuing such a tool.

flore77 · 2016-09-20T12:21:46Z

I will start soon to develop the tool, but I need a name for it, I came up with this ones:

standard-reporter
standard-js-reporter
js-reporter-compliant

Which do you prefer? Or do you have other suggestions.

leobalter · 2016-09-20T13:20:21Z

I like standard-reporter, the other names are fine too

flore77 · 2016-09-20T13:29:12Z

I also like standard-reporter, it is shorter than the others 😃

trentmwillis · 2017-03-19T19:06:33Z

This has been superseded by other PRs. Thanks for the initial work in spiking this out though! It was much appreciated; I am sorry that this PR wound up stalling for so long.

jquerybot added the CLA: Valid label Aug 2, 2016

flore77 force-pushed the adopt-js-reporters-standard branch 2 times, most recently from f421ec3 to 2783fd3 Compare August 2, 2016 21:12

flore77 force-pushed the adopt-js-reporters-standard branch 2 times, most recently from f421ec3 to 0c0848c Compare August 2, 2016 21:27

jquerybot added CLA: Error and removed CLA: Valid labels Aug 2, 2016

jzaefferer reviewed Aug 5, 2016
View reviewed changes

trentmwillis mentioned this pull request Aug 6, 2016

Events: Use EventEmitter model for logging system #882

Closed

JamesMGreene and others added 5 commits August 9, 2016 23:21

Core: QUnit logging system uses EventEmitter

0ed05b1

Fixes qunitjs#422 Closes qunitjs#644

fixup! Core: QUnit logging system uses EventEmitter

a167331

Build: Improve grunt test-on-node task

fb7d73c

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

fixup! Core: QUnit logging system uses EventEmitter

3a502ef

fixup! Core: QUnit logging system uses EventEmitter

2fd7197

flore77 force-pushed the adopt-js-reporters-standard branch from 0d89778 to d24adb0 Compare August 9, 2016 22:41

jzaefferer reviewed Aug 10, 2016
View reviewed changes

flore77 mentioned this pull request Aug 10, 2016

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

Closed

flore77 added 3 commits August 19, 2016 22:42

All: fix linting issues

251aa50

Core: finish adoption of js-reporters events

f522ae6

Tests: add one test more for events

6ea1edb

jzaefferer reviewed Aug 20, 2016
View reviewed changes

src/core.js

import { sourceFromStacktrace } from "./core/stacktrace";

import {Suite} from "js-reporters/lib/Data";

Copy link

Member

jzaefferer Aug 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

All: fix linting

79e4397

Core: finish adoption of js-reporters events

1dfaa16

flore77 force-pushed the adopt-js-reporters-standard branch from be59024 to 1dfaa16 Compare August 21, 2016 20:06

Tests: finish events testing

315c9b3

flore77 added 3 commits August 28, 2016 10:53

Tests: add more event testing

978064f

Core: fix the emitting order of nested modules

32c24ca

Tests: enhance events testing

0a69b20

flore77 force-pushed the adopt-js-reporters-standard branch 2 times, most recently from 160f174 to dfe9486 Compare August 31, 2016 12:30

Tests: add events testing to the browser and browserstack

9fd2e0e

flore77 force-pushed the adopt-js-reporters-standard branch from 41be1fe to 9fd2e0e Compare August 31, 2016 12:37

trentmwillis mentioned this pull request Dec 6, 2016

Tap reporter (prototype yet - please come and discuss) #972

Closed

trentmwillis closed this Mar 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt js reporters standard #1026

Adopt js reporters standard #1026

flore77 commented Aug 2, 2016

platinumazure commented Aug 2, 2016

leobalter commented Aug 2, 2016

flore77 commented Aug 2, 2016 •

edited

Loading

flore77 commented Aug 2, 2016

jzaefferer commented Aug 3, 2016

flore77 commented Aug 4, 2016

leobalter commented Aug 4, 2016

flore77 commented Aug 4, 2016

jzaefferer Aug 5, 2016

jzaefferer Aug 10, 2016

flore77 Aug 10, 2016

trentmwillis commented Aug 6, 2016

leobalter commented Aug 7, 2016

flore77 commented Aug 7, 2016

jzaefferer Aug 10, 2016

flore77 Aug 10, 2016

jzaefferer commented Aug 10, 2016

jzaefferer Aug 20, 2016

jzaefferer commented Aug 20, 2016

flore77 commented Aug 20, 2016

flore77 commented Aug 22, 2016 •

edited

Loading

trentmwillis commented Aug 24, 2016

flore77 commented Aug 24, 2016 •

edited

Loading

flore77 commented Sep 2, 2016

gibson042 commented Sep 7, 2016

flore77 commented Sep 20, 2016

leobalter commented Sep 20, 2016

flore77 commented Sep 20, 2016

trentmwillis commented Mar 19, 2017

		import { sourceFromStacktrace } from "./core/stacktrace";

		import {Suite} from "js-reporters/lib/Data";

Adopt js reporters standard #1026

Adopt js reporters standard #1026

Conversation

flore77 commented Aug 2, 2016

platinumazure commented Aug 2, 2016

leobalter commented Aug 2, 2016

flore77 commented Aug 2, 2016 • edited Loading

flore77 commented Aug 2, 2016

jzaefferer commented Aug 3, 2016

flore77 commented Aug 4, 2016

leobalter commented Aug 4, 2016

flore77 commented Aug 4, 2016

jzaefferer Aug 5, 2016

Choose a reason for hiding this comment

jzaefferer Aug 10, 2016

Choose a reason for hiding this comment

flore77 Aug 10, 2016

Choose a reason for hiding this comment

trentmwillis commented Aug 6, 2016

leobalter commented Aug 7, 2016

flore77 commented Aug 7, 2016

jzaefferer Aug 10, 2016

Choose a reason for hiding this comment

flore77 Aug 10, 2016

Choose a reason for hiding this comment

jzaefferer commented Aug 10, 2016

jzaefferer Aug 20, 2016

Choose a reason for hiding this comment

jzaefferer commented Aug 20, 2016

flore77 commented Aug 20, 2016

flore77 commented Aug 22, 2016 • edited Loading

trentmwillis commented Aug 24, 2016

flore77 commented Aug 24, 2016 • edited Loading

flore77 commented Sep 2, 2016

gibson042 commented Sep 7, 2016

flore77 commented Sep 20, 2016

leobalter commented Sep 20, 2016

flore77 commented Sep 20, 2016

trentmwillis commented Mar 19, 2017

flore77 commented Aug 2, 2016 •

edited

Loading

flore77 commented Aug 22, 2016 •

edited

Loading

flore77 commented Aug 24, 2016 •

edited

Loading