Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Implements QUnit.skip #652

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 17 additions & 10 deletions reporter/html.js
Expand Up @@ -618,7 +618,7 @@ QUnit.log(function( details ) {

QUnit.testDone(function( details ) {
var testTitle, time, testItem, assertList,
good, bad, testCounts,
good, bad, testCounts, skipped,
tests = id( "qunit-tests" );

// QUnit.reset() is deprecated and will be replaced for a new
Expand Down Expand Up @@ -659,17 +659,24 @@ QUnit.testDone(function( details ) {
testTitle.innerHTML += " <b class='counts'>(" + testCounts +
details.assertions.length + ")</b>";

addEvent( testTitle, "click", function() {
toggleClass( assertList, "qunit-collapsed" );
});

time = document.createElement( "span" );
time.className = "runtime";
time.innerHTML = details.runtime + " ms";
if ( details.skipped ) {
addClass( testItem, "skipped" );
skipped = document.createElement( "em" );
skipped.className = "qunit-skipped-label";
skipped.innerHTML = "SKIPPED";
Copy link
Member

Choose a reason for hiding this comment

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

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

testItem.insertBefore( skipped, testTitle );
} else {
addEvent( testTitle, "click", function() {
toggleClass( assertList, "qunit-collapsed" );
});

testItem.className = bad ? "fail" : "pass";
testItem.className = bad ? "fail" : "pass";

testItem.insertBefore( time, assertList );
time = document.createElement( "span" );
time.className = "runtime";
time.innerHTML = details.runtime + " ms";
testItem.insertBefore( time, assertList );
}
});

if ( !defined.document || document.readyState === "complete" ) {
Expand Down
59 changes: 10 additions & 49 deletions src/core.js
Expand Up @@ -117,15 +117,17 @@ QUnit = {
testName: testName,
expected: expected,
async: async,
callback: callback,
module: config.currentModule,
moduleTestEnvironment: config.currentModuleTestEnvironment,
stack: sourceFromStacktrace( 2 )
callback: callback
});

if ( !validTest( test ) ) {
return;
}
test.queue();
},

skip: function( testName ) {
var test = new Test({
testName: testName,
skip: true
});

test.queue();
},
Expand Down Expand Up @@ -447,7 +449,7 @@ window.onerror = function( error, filePath, linerNr ) {
} else {
QUnit.test( "global failure", extend(function() {
QUnit.pushFailure( error, filePath + ":" + linerNr );
}, { validTest: validTest } ) );
}, { validTest: true } ) );
}
return false;
}
Expand Down Expand Up @@ -481,47 +483,6 @@ function done() {
});
}

/** @return Boolean: true if this test should be ran */
function validTest( test ) {
var include,
filter = config.filter && config.filter.toLowerCase(),
module = config.module && config.module.toLowerCase(),
fullName = ( test.module + ": " + test.testName ).toLowerCase();

// Internally-generated tests are always valid
if ( test.callback && test.callback.validTest === validTest ) {
delete test.callback.validTest;
return true;
}

if ( config.testNumber.length > 0 ) {
if ( inArray( test.testNumber, config.testNumber ) < 0 ) {
return false;
}
}

if ( module && ( !test.module || test.module.toLowerCase() !== module ) ) {
return false;
}

if ( !filter ) {
return true;
}

include = filter.charAt( 0 ) !== "!";
if ( !include ) {
filter = filter.slice( 1 );
}

// If the filter matches, we need to honour include
if ( fullName.indexOf( filter ) !== -1 ) {
return include;
}

// Otherwise, do the opposite
return !include;
}

// Doesn't support IE6 to IE9
// See also https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error/Stack
function extractStacktrace( e, offset ) {
Expand Down
19 changes: 19 additions & 0 deletions src/qunit.css
Expand Up @@ -99,6 +99,10 @@
cursor: pointer;
}

#qunit-tests li.skipped strong {
cursor: default;
}

#qunit-tests li a {
padding: 0.5em;
color: #C2CCD1;
Expand Down Expand Up @@ -211,6 +215,21 @@

#qunit-banner.qunit-fail { background-color: #EE5757; }

/*** Skipped tests */

#qunit-tests .skipped {
background-color: #EBECE9;
}

#qunit-tests .qunit-skipped-label {
background-color: #F4FF77;
display: inline-block;
font-style: normal;
color: #366097;
line-height: 1.8em;
padding: 0 0.5em;
margin: -0.4em 0.4em -0.4em 0;
}

/** Result */

Expand Down
65 changes: 64 additions & 1 deletion src/test.js
@@ -1,8 +1,20 @@
function Test( settings ) {
extend( this, settings );
this.assert = new Assert( this );
this.assertions = [];
this.testNumber = ++Test.count;
this.module = config.currentModule;
this.moduleTestEnvironment = config.currentModuleTestEnvironment;
this.stack = sourceFromStacktrace( 3 );

if ( settings.skip ) {

// Skipped tests will fully ignore any sent callback
this.callback = function() {};
this.async = false;
this.expected = 0;
} else {
this.assert = new Assert( this );
}
}

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

// hooks are also ignored on skipped tests
Copy link
Member

Choose a reason for hiding this comment

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

Uppcase first char in line comments

if ( this.skip ) {
return hooks;
}

if ( QUnit.objectType( config[ handler ] ) === "function" ) {
hooks.push( this.queueHook( config[ handler ], handler ) );
}
Expand Down Expand Up @@ -139,6 +156,7 @@ Test.prototype = {
runLoggingCallbacks( "testDone", {
name: this.testName,
module: this.module,
skipped: !!this.skip,
Copy link
Member

Choose a reason for hiding this comment

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

Related to js-reporters/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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

failed: bad,
passed: this.assertions.length - bad,
total: this.assertions.length,
Expand All @@ -159,6 +177,10 @@ Test.prototype = {
var bad,
test = this;

if ( !this.valid() ) {
return;
}

function run() {

// each of these can by async
Expand Down Expand Up @@ -250,6 +272,47 @@ Test.prototype = {
result: false,
message: message
});
},

/** @return Boolean: true if this test should be ran */
valid: function() {
var include,
filter = config.filter && config.filter.toLowerCase(),
module = config.module && config.module.toLowerCase(),
fullName = ( this.module + ": " + this.testName ).toLowerCase();

// Internally-generated tests are always valid
if ( this.callback && this.callback.validTest ) {
delete this.callback.validTest;
Copy link
Member

Choose a reason for hiding this comment

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

This used to be a function reference, removing it made a little bit of sense for memory management. Now its just a boolean, so we can just leave the property. If that's correct, let's remove this line.

return true;
}

if ( config.testNumber.length > 0 ) {
if ( inArray( this.testNumber, config.testNumber ) < 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, let's merge these two if statements.

return false;
}
}

if ( module && ( !this.module || this.module.toLowerCase() !== module ) ) {
return false;
}

if ( !filter ) {
return true;
}

include = filter.charAt( 0 ) !== "!";
if ( !include ) {
filter = filter.slice( 1 );
}

// If the filter matches, we need to honour include
if ( fullName.indexOf( filter ) !== -1 ) {
return include;
}

// Otherwise, do the opposite
return !include;
}
};

Expand Down
3 changes: 2 additions & 1 deletion test/logs.js
Expand Up @@ -150,7 +150,8 @@ QUnit.test( "test2", function( assert ) {
failed: 0,
passed: 17,
total: 17,
testNumber: 1
testNumber: 1,
skipped: false
}, "testDone context" );
assert.deepEqual( testContext, {
module: "logs1",
Expand Down
22 changes: 22 additions & 0 deletions test/test.js
Expand Up @@ -438,3 +438,25 @@ QUnit.test( "mod2", function( assert ) {
assert.mod2( 2, 0, "2 % 2 == 0" );
assert.mod2( 3, 1, "3 % 2 == 1" );
});

QUnit.module( "QUnit.skip", {
beforeEach: function( assert ) {

// skip test hooks for skipped tests
assert.ok( false, "skipped function" );
throw "Error";
},
afterEach: function( assert ) {
assert.ok( false, "skipped function" );
throw "Error";
}
});

QUnit.skip( "test blocks are skipped", function( assert ) {

// this test callback won't run, even with broken code
assert.expect( 1000 );
throw "Error";
});

QUnit.skip( "no function" );