Core: Implements QUnit.skip #652

leobalter · 2014-09-04T16:54:33Z

Fixes #637
Fixes #434

JamesMGreene · 2014-09-04T17:28:32Z

LGTM.

I would also like to see the HTML Reporter be updated to give skipped tests a different visual treatment.

Once we get the core HTML Reporter markup/CSS taken care of, we should also file new issues with all of the various QUnit HTML Reporter theme repos as well:

leobalter · 2014-09-04T17:56:22Z

I can pass a value on the testDone callback

gibson042 · 2014-09-04T19:06:23Z

Agreed on the reporter changes. I'd also like a test covering the QUnit.skip("no function") case.

gibson042 · 2014-09-04T19:13:30Z

src/test.js

+		this.testName = "SKIPPED - " + this.testName;
+		for ( method in Assert.prototype ) {
+			this.assert[ method ] = skippedAssertion;
+		}


Is this necessary? And if yes, would the code make more sense elsewhere with something like this.assert = new Assert( settings.skip ? null : this ) here?

this is necessary to prevent assertions inside the module hooks (beforeEach/afterEach), on the skipped tests.

I'm stubbing every method defined on Assert's prototype and I believe it's the Test's responsibility to handle if it's a skipped one or not.

I agree that properly reacting to settings.skip is the Test's responsibility, but manually faking a dummy object similar to an Assert but not instanceof Assert rather than delegating the implementation seems kludgy.

One alternative may be to instead short-circuit the Test#hooks call to not retrieve any hooks if the test is marked as skip. This seems reasonable to me but that reasonability really depends on consumer usage.

leobalter · 2014-09-04T20:06:18Z

@JamesMGreene @gibson042: Implemented the reporter logging and also the suggested test.

JamesMGreene · 2014-09-05T01:34:14Z

src/test.js

@@ -139,6 +152,7 @@ Test.prototype = {
 		runLoggingCallbacks( "testDone", {
 			name: this.testName,
 			module: this.module,
+			skipped: !!this.skip,


Related to qunitjs/js-reporters#4: I'm thinking we'll probably end up standardizing this to be a more generic property like status: "skipped". Not sure if we want to pursue that presumed direction yet in this PR or just leave this QUnit-specific for now.

I think we should just proceed by leaving this as a QUnit-specific thing for now and we can update in QUnit v2.0 if we decide on a more standard format going forward in collaboration with the other testing frameworks.

yes, I've preferred to keep it as it is before it's definition on the js-reporter.

leobalter · 2014-09-05T11:28:10Z

src/test.js

+	skippedAssertion = function() {};
+
+	if ( settings.skip ) {
+		this.testName = "SKIPPED - " + this.testName;


as skipped tests blocks - and respective hooks - aren't called anymore, there's no more need to fake the Assert object.

cc @gibson042 @JamesMGreene

follow back to #652 (comment)

Excellent! skippedAssertion can now be dropped as well.

leobalter · 2014-09-05T15:21:02Z

Thanks @gibson042 and @JamesMGreene. Fixed the code and rebased with master.

JamesMGreene · 2014-09-05T15:27:02Z

LGTM! 👍

JamesMGreene · 2014-09-05T16:21:38Z

@jzaefferer @Krinkle @scottgonzalez: Any desire to review this PR further before we merge it?

jzaefferer · 2014-09-05T19:00:17Z

test/test.js

+	throw "Error";
+});
+
+QUnit.skip ( "no function" );


jscs doesn't detect the extra space here. Let's fix this and report the missing check.

jzaefferer · 2014-09-05T19:12:01Z

Reviewed this in detail, found a few issues. Apart from that it would be good to outline what API docs need to be written or updated. Let's put that in the description of the PR.

jzaefferer · 2014-09-05T19:23:16Z

src/core.js

+			testName: testName,
+			expected: 0,
+			async: false,
+			callback: function() {},


This deserves a line comment, I missed it on first review.

Though if we can move the default properties inside the constructor, the differences to the call in QUnit.test should be much more obvious.

I also missed it the first time through, took a second pass to find it.

Krinkle · 2014-09-07T20:50:27Z

Design idea for html reporter:

Not sure about the <em>.

Krinkle · 2014-09-07T20:54:44Z

We should also include the number of skipped tests in the summary atop the html reporter.

Tests completed in 1782 milliseconds.
331 assertions of 331 passed, 0 failed.

Maybe add something like

2 skipped tests.

That way they're discoverable without having to scan the page manually.

PHPUnit, for example, reports:

OK (but with incomplete or skipped tests)
Tests: 8693, Assertions: 574690, Skipped: 8.

JamesMGreene · 2014-09-08T13:11:21Z

I think the background color of the cell should still be different for skipped tests but adding the yellow bubble is a nice additional visual treatment.

I also agree about including the number of skipped tests in the summary.

leobalter · 2014-09-08T15:17:05Z

Fixed after reviews, still missing:

jzaefferer · 2014-09-08T15:18:42Z

Adding the bubble along with a different background color, while not modifying the test name sounds good.

Since skipped tests don't run assertions, we can't actually tell how many assertions were skipped. Mixing assertions and tests in the summary sounds like a bad idea. I'm okay with landing this without changes to the summary.

leobalter · 2014-09-08T15:19:42Z

About the bubble label: I prefer to change only the background color instead of inserting a bubble. As we are using the colors to indicate the test status and skipped is a new one.

leobalter · 2014-09-08T15:24:01Z

I like the idea to include the number of skipped tests on the summary, as long as they are not on the sum of the passed tests.

Also logs skipped tests on testDone callback that are handled on the html reporter Fixes qunitjs#637 Fixes qunitjs#434

leobalter · 2014-09-08T15:50:58Z

After talking with @jzaefferer, I'm trying a new suggestion: grey background for the skipped line with the yellow bubble. Runtime is also not inserted (there's not test to run there). Rerun link was intentionally kept.

leobalter · 2014-09-08T15:55:55Z

As we summarize only the passed and failed assertions on the reporter, I prefer to address it on another issue/PR.

jzaefferer · 2014-09-08T16:00:48Z

reporter/html.js

+		addClass( testItem, "skipped" );
+		skipped = document.createElement( "em" );
+		skipped.className = "qunit-skipped-label";
+		skipped.innerHTML = "SKIPPED";


Make this "Skipped", add a CSS rule to show it uppercase. Allows themes to override that.

jzaefferer · 2014-09-08T16:30:39Z

test/logs.js

+	assert.expect( 3 );
+
+	assert.ok( testDoneContext.runtime >= 0, "logs runtime" );
+	assert.ok( testDoneContext.duration >= 0, "logs deprecated duration" );


Let's remove these two assertions. We don't care about the runtime of skipped tests. Its okay to have them there, but we don't need to test for that.

jzaefferer · 2014-09-08T16:52:35Z

src/test.js

@@ -102,6 +114,11 @@ Test.prototype = {
 	hooks: function( handler ) {
 		var hooks = [];

+		// hooks are also ignored on skipped tests


Uppcase first char in line comments

jzaefferer · 2014-09-08T17:13:04Z

Reviewed everything once more, looks great to me. Would be nice to get a nod-off from @JamesMGreene or @Krinkle as well.

Krinkle · 2014-09-10T01:34:34Z

Good call on applying a change to the background of the row as well. The grey and yellow look good to me:

(RE: 0122c75) The ALL CAPS label text is a bit excessive though. Title Case doesn't seem appropriate either. Perhaps lowercase will fit in nicely with the overall friendly interface?

JamesMGreene · 2014-09-10T19:17:33Z

I'd say definitely not TitleCase for the "SKIPPED" bubble. I'm fine with either all uppercase or lowercase.

leobalter · 2014-09-10T22:02:35Z

Done, lowercase "skipped" is now the default.

Also other reporter can change it through CSS's text-transform

Fixes qunitjs#75 Ref qunitjs/qunit#637 Ref qunitjs/qunit#652

Ref qunitjs/qunit#637 Ref qunitjs/qunit#652 Fixes #75 Closes #77

gibson042 reviewed Sep 4, 2014
View reviewed changes

leobalter force-pushed the 637-skip branch from ac8dbf8 to 4ff6765 Compare September 4, 2014 20:04

JamesMGreene reviewed Sep 5, 2014
View reviewed changes

leobalter force-pushed the 637-skip branch from 4ff6765 to dfe5be2 Compare September 5, 2014 11:25

leobalter reviewed Sep 5, 2014
View reviewed changes

leobalter force-pushed the 637-skip branch 2 times, most recently from 8189558 to 9dd6a77 Compare September 5, 2014 15:20

jzaefferer reviewed Sep 5, 2014
View reviewed changes

leobalter force-pushed the 637-skip branch from 9dd6a77 to dea89c1 Compare September 8, 2014 15:17

Core: Implements QUnit.skip

43dbed4

Also logs skipped tests on testDone callback that are handled on the html reporter Fixes qunitjs#637 Fixes qunitjs#434

leobalter force-pushed the 637-skip branch from dea89c1 to 43dbed4 Compare September 8, 2014 15:47

jzaefferer reviewed Sep 8, 2014
View reviewed changes

leobalter mentioned this pull request Sep 8, 2014

Document QUnit.skip qunitjs/api.qunitjs.com#75

Closed

jzaefferer reviewed Sep 8, 2014
View reviewed changes

leobalter added 2 commits September 8, 2014 13:38

Logs: Tests logs for skipped tests

e241917

Test: Few improvs to the code

9bff489

leobalter force-pushed the 637-skip branch from 345a96e to 7f8f5a9 Compare September 8, 2014 16:43

jzaefferer reviewed Sep 8, 2014
View reviewed changes

Reporter: Uppercase skipped labels via CSS

0122c75

leobalter force-pushed the 637-skip branch from 7f8f5a9 to 0122c75 Compare September 8, 2014 16:52

leobalter added 2 commits September 8, 2014 13:55

Test: Minor fixes

e172590

Test: Whitepace

7b11fdb

Reporter: Lowercase on skipped label

4077358

leobalter closed this in 3f08a1a Sep 11, 2014

jzaefferer mentioned this pull request Sep 11, 2014

API: Documents QUnit.skip qunitjs/api.qunitjs.com#77

Closed

leobalter added a commit to leobalter/api.qunitjs.com that referenced this pull request Sep 12, 2014

API: Documents QUnit.skip

a59f9c0

Fixes qunitjs#75 Ref qunitjs/qunit#637 Ref qunitjs/qunit#652

JamesMGreene mentioned this pull request Sep 29, 2014

Create an inconclusive assertion #326

Closed

leobalter added a commit to qunitjs/api.qunitjs.com that referenced this pull request Dec 3, 2014

Entries: Document QUnit.skip

cae9f27

Ref qunitjs/qunit#637 Ref qunitjs/qunit#652 Fixes #75 Closes #77

leobalter deleted the 637-skip branch October 27, 2015 15:50

Core: Implements QUnit.skip #652

Core: Implements QUnit.skip #652

Conversation

leobalter commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

leobalter commented Sep 4, 2014

gibson042 commented Sep 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Sep 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Sep 5, 2014

JamesMGreene commented Sep 5, 2014

JamesMGreene commented Sep 5, 2014

Choose a reason for hiding this comment

jzaefferer commented Sep 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krinkle commented Sep 7, 2014

Krinkle commented Sep 7, 2014

JamesMGreene commented Sep 8, 2014

leobalter commented Sep 8, 2014

jzaefferer commented Sep 8, 2014

leobalter commented Sep 8, 2014

leobalter commented Sep 8, 2014

leobalter commented Sep 8, 2014

leobalter commented Sep 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzaefferer commented Sep 8, 2014

Krinkle commented Sep 10, 2014

JamesMGreene commented Sep 10, 2014

leobalter commented Sep 10, 2014