Skip to content

Decouple HTML reporter from core #603

Merged
merged 6 commits into from Jul 22, 2014

5 participants

@leobalter
jQuery Foundation member

This is the first step needed to set the reporter interface.

There are still a lot of things to do, but I already want to share to allow earlier reviews, because it's necessarily huge.

@leobalter leobalter changed the title from [WIP] Decouple the HTML reporter to Detach the HTML reporter Jul 1, 2014
@leobalter
jQuery Foundation member

It's finally done.

I know it's very hard to review this but it's really necessary for further features like the Reporter Interface, a console reporter, etc.

It also fixes the remaining issue of #583, the HTML reporter sets each assertion separately and it's outputted on the respective test block.

And this also allow us to make an extra build for non-Browser environments, if we use some console reporter.

It respects the old API, without removing anything or even the HTML report format.

It's also very tested on all supported browsers.

Now I need some of your time to test and review this, please. @jzaefferer, @Krinkle, @JamesMGreene.

@jzaefferer
jQuery Foundation member

I just checked out the branch and did the very basics (run grunt, run testsuite in Chrome, run browserstack-runner).

I then merged the "async-future" branch (#583) and checked the output in the browser. The assertions that would otherwise leak or disappear now show up in their respective tests. The only concern I have there that the first test now fails with expect saying the it expected one assertion, but zero ran, then afterwards the passed ok assertion shows up. That should probably fail in some way, since it runs after the test is already done.

I then started reviewing the changes in test/. The addition of the testNumber property is an API change that we need to document, though I don't see any backwards compatibility concerns with that.

Removing the testAfterDone block looks good to me, since the logs test should cover that already.

I'm going to review the html-reporter next, ignoring the possibility that code was just moved from it's previous location, since that is hard to tell from the diff. That might produce a lot of comments, so I'm going to finish this one before that.

From the outside, this looks great. It will likely need some more time to finish. I will likely not be available for the rest of this month, so I hope @JamesMGreene @Krinkle or @scottgonzalez can help Leo to land this.

@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
@@ -0,0 +1,579 @@
+(function() {
+
+// Don't load the HTML Reporter on non-Browser environments
+if ( typeof window === "undefined" ) { return; }
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Missing line breaks.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ * @param {string} type
+ * @param {Function} fn
+ */
+function addEvent( elem, type, fn ) {
+ if ( elem.addEventListener ) {
+
+ // Standards-based browsers
+ elem.addEventListener( type, fn, false );
+ } else if ( elem.attachEvent ) {
+
+ // support: IE <9
+ elem.attachEvent( "on" + type, fn );
+ } else {
+
+ // Caller must ensure support for event listeners is present
+ throw new Error( "addEvent() was called in a context without event listener support" );
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

jQuery Core 1.x just does nothing when neither is supported. Since this code shouldn't run anymore when there is no DOM, we could remove this else/throws branch.

@leobalter
jQuery Foundation member
leobalter added a note Jul 3, 2014

good catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+}
+
+/**
+ * @param {Array|NodeList} elems
+ * @param {string} type
+ * @param {Function} fn
+ */
+function addEvents( elems, type, fn ) {
+ var i = elems.length;
+ while ( i-- ) {
+ addEvent( elems[ i ], type, fn );
+ }
+}
+
+function hasClass( elem, name ) {
+ return ( " " + elem.className + " " ).indexOf( " " + name + " " ) > -1;
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

jQuery Core compares to >= 0, I usually use !== -1. I wonder if that's worth making consistent (aka putting it into the style guide).

@leobalter
jQuery Foundation member
leobalter added a note Jul 3, 2014

this is exactly the same hasClass function, I just moved this (copy/paste) to simplify review. Anyway, it's a good change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+
+function addClass( elem, name ) {
+ if ( !hasClass( elem, name ) ) {
+ elem.className += ( elem.className ? " " : "" ) + name;
+ }
+}
+
+function removeClass( elem, name ) {
+ var set = " " + elem.className + " ";
+
+ // Class name may appear multiple times
+ while ( set.indexOf( " " + name + " " ) > -1 ) {
+ set = set.replace( " " + name + " ", " " );
+ }
+
+ // If possible, trim it for prettiness, but not necessarily
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

The "but not necessarily" doesn't make sense, since the code always trims. Could change this "trim for prettiness" or just remove it.

@leobalter
jQuery Foundation member
leobalter added a note Jul 3, 2014

It isn't even necessary to have this. For performance I would even drop the trim at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ set = set.replace( " " + name + " ", " " );
+ }
+
+ // If possible, trim it for prettiness, but not necessarily
+ elem.className = typeof set.trim === "function" ? set.trim() : set.replace( /^\s+|\s+$/g, "" );
+}
+
+function id( name ) {
+ return defined.document && document.getElementById && document.getElementById( name );
+}
+
+function htmlReporterLoad() {
+
+ // Initialize the config, saving the execution queue
+ var banner, filter, i, j, label, len, main, ol, toolbar, val, selection,
+ urlConfigContainer, moduleFilter, userAgent,
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Too many variables. Maybe refactor this function into a few smaller ones, e.g. each creating some part of the interface.

@leobalter
jQuery Foundation member
leobalter added a note Jul 3, 2014

anotther copy/paste from src/core.js, it was the QUnit.load callback, ref: https://github.com/jquery/qunit/pull/603/files#diff-7eb52b366866677666470e019283c8eaL477

I agree 100% we can refactor this, but I prefer to address this one in a new issue/PR, for now it's just detaching.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

It's done now. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+
+function id( name ) {
+ return defined.document && document.getElementById && document.getElementById( name );
+}
+
+function htmlReporterLoad() {
+
+ // Initialize the config, saving the execution queue
+ var banner, filter, i, j, label, len, main, ol, toolbar, val, selection,
+ urlConfigContainer, moduleFilter, userAgent,
+ numModules = 0,
+ moduleNames = [],
+ moduleFilterHtml = "",
+ urlConfigHtml = "";
+
+ len = config.urlConfig.length;
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

This doesn't have to be separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ escapeText( config[ val.id ] ) +
+ "</option>";
+ }
+ urlConfigHtml += "</select>";
+ }
+ }
+ for ( i in config.modules ) {
+ if ( config.modules.hasOwnProperty( i ) ) {
+ moduleNames.push( i );
+ }
+ }
+ numModules = moduleNames.length;
+ moduleNames.sort(function( a, b ) {
+ return a.localeCompare( b );
+ });
+ moduleFilterHtml += "<label for='qunit-modulefilter'>Module: </label><select id='qunit-modulefilter' name='modulefilter'><option value='' " +
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

This and two lines close by below are too long (must be <100).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ moduleFilterHtml += "<option value='" + escapeText( encodeURIComponent( moduleNames[ i ] ) ) + "' " +
+ ( config.module === moduleNames[ i ] ? "selected='selected'" : "" ) +
+ ">" + escapeText( moduleNames[ i ] ) + "</option>";
+ }
+ moduleFilterHtml += "</select>";
+
+ // `userAgent` initialized at top of scope
+ userAgent = id( "qunit-userAgent" );
+ if ( userAgent ) {
+ userAgent.innerHTML = navigator.userAgent;
+ }
+
+ // `banner` initialized at top of scope
+ banner = id( "qunit-header" );
+ if ( banner ) {
+ banner.innerHTML = "<a href='" + QUnit.url( { filter: undefined, module: undefined, testNumber: undefined } ) + "'>" + banner.innerHTML + "</a> ";
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Line too long; no space between ( and {, should be QUnit.url({ filter:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ return defined.document && document.getElementById && document.getElementById( name );
+}
+
+function htmlReporterLoad() {
+
+ // Initialize the config, saving the execution queue
+ var banner, filter, i, j, label, len, main, ol, toolbar, val, selection,
+ urlConfigContainer, moduleFilter, userAgent,
+ numModules = 0,
+ moduleNames = [],
+ moduleFilterHtml = "",
+ urlConfigHtml = "";
+
+ len = config.urlConfig.length;
+
+ for ( i = 0; i < len; i++ ) {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

All this, which creates urlConfigHtml, and the block below, which creates moduleFilterHtml, is only need when the toolbar is rendered (checked below). Should extract these into separate functions are run them as needed, if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+
+ // `ol` initialized at top of scope
+ ol = id( "qunit-tests" );
+ ol.className = ol.className + " hidepass";
+ }
+ toolbar.appendChild( filter );
+
+ // `label` initialized at top of scope
+ label = document.createElement( "label" );
+ label.setAttribute( "for", "qunit-filter-pass" );
+ label.setAttribute( "title", "Only show tests and assertions that fail. Stored in sessionStorage." );
+ label.innerHTML = "Hide passed tests";
+ toolbar.appendChild( label );
+
+ urlConfigContainer = document.createElement( "span" );
+ urlConfigContainer.innerHTML = urlConfigHtml;
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Could make this a function call urlConfigHtml( ... ), passing along the necessary variables. That should reduce the amount of variables in this scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ target.defaultValue || true :
+ undefined;
+ window.location = QUnit.url( params );
+ });
+ addEvents( urlConfigContainer.getElementsByTagName( "select" ), "change", function( event ) {
+ var params = {},
+ target = event.target || event.srcElement;
+ params[ target.name ] = target.options[ target.selectedIndex ].value || undefined;
+ window.location = QUnit.url( params );
+ });
+ toolbar.appendChild( urlConfigContainer );
+
+ if ( numModules > 1 ) {
+ moduleFilter = document.createElement( "span" );
+ moduleFilter.setAttribute( "id", "qunit-modulefilter-container" );
+ moduleFilter.innerHTML = moduleFilterHtml;
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Same as above. Not sure about the event handlers, maybe pass in the element and have the helper also add the events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ if ( filter.checked ) {
+ ol.className = ol.className + " hidepass";
+ } else {
+ tmp = " " + ol.className.replace( /[\n\t\r]/g, " " ) + " ";
+ ol.className = tmp.replace( / hidepass /, " " );
+ }
+ if ( defined.sessionStorage ) {
+ if ( filter.checked ) {
+ sessionStorage.setItem( "qunit-filter-passed-tests", "true" );
+ } else {
+ sessionStorage.removeItem( "qunit-filter-passed-tests" );
+ }
+ }
+ });
+
+ if ( config.hidepassed || defined.sessionStorage && sessionStorage.getItem( "qunit-filter-passed-tests" ) ) {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Line too long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ params[ target.name ] = target.options[ target.selectedIndex ].value || undefined;
+ window.location = QUnit.url( params );
+ });
+ toolbar.appendChild( urlConfigContainer );
+
+ if ( numModules > 1 ) {
+ moduleFilter = document.createElement( "span" );
+ moduleFilter.setAttribute( "id", "qunit-modulefilter-container" );
+ moduleFilter.innerHTML = moduleFilterHtml;
+ addEvent( moduleFilter.lastChild, "change", function() {
+ var selectBox = moduleFilter.getElementsByTagName( "select" )[ 0 ],
+ selectedModule = decodeURIComponent( selectBox.options[ selectBox.selectedIndex ].value );
+
+ window.location = QUnit.url({
+ module: ( selectedModule === "" ) ? undefined : selectedModule,
+ // Remove any existing filters
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Blank lines before comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ addEvent( moduleFilter.lastChild, "change", function() {
+ var selectBox = moduleFilter.getElementsByTagName( "select" )[ 0 ],
+ selectedModule = decodeURIComponent( selectBox.options[ selectBox.selectedIndex ].value );
+
+ window.location = QUnit.url({
+ module: ( selectedModule === "" ) ? undefined : selectedModule,
+ // Remove any existing filters
+ filter: undefined,
+ testNumber: undefined
+ });
+ });
+ toolbar.appendChild( moduleFilter );
+ }
+ }
+
+ // `main` initialized at top of scope
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

We can remove these comments once the whole function is more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ filter: undefined,
+ testNumber: undefined
+ });
+ });
+ toolbar.appendChild( moduleFilter );
+ }
+ }
+
+ // `main` initialized at top of scope
+ main = id( "qunit-fixture" );
+ if ( main ) {
+ config.fixture = main.innerHTML;
+ }
+}
+
+// Old implementation from QUnit.init()
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Drop these.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ toolbar.appendChild( moduleFilter );
+ }
+ }
+
+ // `main` initialized at top of scope
+ main = id( "qunit-fixture" );
+ if ( main ) {
+ config.fixture = main.innerHTML;
+ }
+}
+
+// Old implementation from QUnit.init()
+// HTML Reporter initialization and load
+QUnit.begin(function() {
+ var tests, banner, result,
+ qunit = id( "qunit" );
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Missing indent

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+
+ if ( qunit ) {
+ qunit.innerHTML =
+ "<h1 id='qunit-header'>" + escapeText( document.title ) + "</h1>" +
+ "<h2 id='qunit-banner'></h2>" +
+ "<div id='qunit-testrunner-toolbar'></div>" +
+ "<h2 id='qunit-userAgent'></h2>" +
+ "<ol id='qunit-tests'></ol>";
+ }
+
+ tests = id( "qunit-tests" );
+ banner = id( "qunit-banner" );
+ result = id( "qunit-testresult" );
+
+ if ( tests ) {
+ tests.innerHTML = "";
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

This is pointless, since the innerHTML property is overwritten again below, using the same condition as on the previous line.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

it's the same condition, but innerHTML is not overwritten

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ }
+
+ if ( result ) {
+ result.parentNode.removeChild( result );
+ }
+
+ if ( tests ) {
+ result = document.createElement( "p" );
+ result.id = "qunit-testresult";
+ result.className = "result";
+ tests.parentNode.insertBefore( result, tests );
+ result.innerHTML = "Running...<br/>&nbsp;";
+ }
+
+ // Old html implementation from QUnit.load
+ htmlReporterLoad();
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Drop the comment, maybe rename the function. Or inline the function after it got reduced to just a few lines, if possible.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ var selectBox = moduleFilter.getElementsByTagName( "select" )[ 0 ],
+ selectedModule = decodeURIComponent( selectBox.options[ selectBox.selectedIndex ].value );
+
+ window.location = QUnit.url({
+ module: ( selectedModule === "" ) ? undefined : selectedModule,
+ // Remove any existing filters
+ filter: undefined,
+ testNumber: undefined
+ });
+ });
+ toolbar.appendChild( moduleFilter );
+ }
+ }
+
+ // `main` initialized at top of scope
+ main = id( "qunit-fixture" );
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Rename to fixture to be consistent with it's id and the usage in testDone below.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ tests = id( "qunit-tests" ),
+ html = [
+ "Tests completed in ",
+ details.runtime,
+ " milliseconds.<br/>",
+ "<span class='passed'>",
+ details.passed,
+ "</span> assertions of <span class='total'>",
+ details.total,
+ "</span> passed, <span class='failed'>",
+ details.failed,
+ "</span> failed."
+ ].join( "" );
+
+ if ( banner ) {
+ banner.className = ( details.failed ? "qunit-fail" : "qunit-pass" );
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Remove the unnecessary parens.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ }
+
+ if ( config.altertitle && defined.document && document.title ) {
+
+ // show ✖ for good, ✔ for bad suite result in title
+ // use escape sequences in case file gets loaded with non-utf-8-charset
+ document.title = [
+ ( details.failed ? "\u2716" : "\u2714" ),
+ document.title.replace( /^[\u2714\u2716] /i, "" )
+ ].join( " " );
+ }
+
+ // clear own sessionStorage items if all tests passed
+ if ( config.reorder && defined.sessionStorage && details.failed === 0 ) {
+
+ // `key` & `i` initialized at top of scope
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Let's drop this comment, they're not that hard to find.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ result.parentNode.removeChild( result );
+ }
+
+ if ( tests ) {
+ result = document.createElement( "p" );
+ result.id = "qunit-testresult";
+ result.className = "result";
+ tests.parentNode.insertBefore( result, tests );
+ result.innerHTML = "Running...<br/>&nbsp;";
+ }
+
+ // Old html implementation from QUnit.load
+ htmlReporterLoad();
+});
+
+QUnit.testDone(function() {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Merge this with the testDone below.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+
+ nameHtml += "<span class='test-name'>" + escapeText( name ) + "</span>";
+
+ return nameHtml;
+}
+
+QUnit.testStart(function( details ) {
+ var a, b, li, running, assertList,
+ name = getNameHtml( details.name, details.module ),
+ tests = id( "qunit-tests" );
+
+ if ( tests ) {
+ b = document.createElement( "strong" );
+ b.innerHTML = name;
+
+ // `a` initialized at top of scope
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Remove

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ return nameHtml;
+}
+
+QUnit.testStart(function( details ) {
+ var a, b, li, running, assertList,
+ name = getNameHtml( details.name, details.module ),
+ tests = id( "qunit-tests" );
+
+ if ( tests ) {
+ b = document.createElement( "strong" );
+ b.innerHTML = name;
+
+ // `a` initialized at top of scope
+ a = document.createElement( "a" );
+ a.innerHTML = "Rerun";
+ a.href = QUnit.url( { testNumber: details.testNumber } );
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

No space between parens and curly braces, on both sides (as above).

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ li.appendChild( a );
+ li.className = "running";
+ li.id = "qunit-test-output" + details.testNumber;
+
+ assertList = document.createElement( "ol" );
+ assertList.className = "qunit-assert-list";
+
+ li.appendChild( assertList );
+
+ tests.appendChild( li );
+ }
+
+ running = id( "qunit-testresult" );
+
+ if ( running ) {
+ running.innerHTML = "Running: <br/>" + name;
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Remove the slash, empty elements don't need that (we're not writing XML).

@leobalter
jQuery Foundation member
leobalter added a note Jul 3, 2014

I have an stupid question: removing the slash wouldn't break IE6?

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

It was removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ assertList.className = "qunit-assert-list";
+
+ li.appendChild( assertList );
+
+ tests.appendChild( li );
+ }
+
+ running = id( "qunit-testresult" );
+
+ if ( running ) {
+ running.innerHTML = "Running: <br/>" + name;
+ }
+
+});
+
+// Register assertions
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

This comment seems rather unhelpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+});
+
+// Register assertions
+QUnit.log(function( details ) {
+ var assertList, assertLi,
+ message, expected, actual,
+ testItem = id( "qunit-test-output" + details.testNumber );
+
+ if ( !testItem ) {
+ return;
+ }
+
+ message = escapeText( details.message ) || ( details.result ? "okay" : "failed" );
+ message = "<span class='test-message'>" + message + "</span>";
+
+ // pushFailure doesn't log details.expected
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Maybe replace "log" with "provide"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+// Register assertions
+QUnit.log(function( details ) {
+ var assertList, assertLi,
+ message, expected, actual,
+ testItem = id( "qunit-test-output" + details.testNumber );
+
+ if ( !testItem ) {
+ return;
+ }
+
+ message = escapeText( details.message ) || ( details.result ? "okay" : "failed" );
+ message = "<span class='test-message'>" + message + "</span>";
+
+ // pushFailure doesn't log details.expected
+ // when it calls, it's implicit to also not show expected and diff stuff
+ if ( !details.result && hasOwn.call( details, "expected" ) ) {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Can't you just check for details.expected !== undefined?

@leobalter
jQuery Foundation member
leobalter added a note Jul 3, 2014

details.expected can be undefined.

I'm intentionally checking for a property existence.

I'll insert a comment explaining there.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ good, bad,
+ nameHtml = getNameHtml( details.name, details.module ),
+ tests = id( "qunit-tests" );
+
+ if ( !tests ) {
+ return;
+ }
+
+ testItem = id( "qunit-test-output" + details.testNumber );
+ assertList = testItem.getElementsByTagName( "ol" )[ 0 ];
+
+ good = details.passed;
+ bad = details.failed;
+
+ // store result when possible
+ if ( QUnit.config.reorder && defined.sessionStorage ) {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Can use config.reorder, that's in scope already.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ bad = details.failed;
+
+ // store result when possible
+ if ( QUnit.config.reorder && defined.sessionStorage ) {
+ if ( bad ) {
+ sessionStorage.setItem( "qunit-test-" + details.module + "-" + details.name, bad );
+ } else {
+ sessionStorage.removeItem( "qunit-test-" + details.module + "-" + details.name );
+ }
+ }
+
+ if ( bad === 0 ) {
+ addClass( assertList, "qunit-collapsed" );
+ }
+
+ // `b` initialized at top of scope
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Remove. Also the ones below.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

With smaller functions and JSHint, I safely removed these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ addClass( assertList, "qunit-collapsed" );
+ }
+
+ // `b` initialized at top of scope
+ b = document.createElement( "strong" );
+ b.innerHTML = nameHtml +
+ " <b class='counts'>(<b class='failed'>" + bad + "</b>, <b class='passed'>" + good +
+ "</b>, " + details.assertions.length + ")</b>";
+
+ addEvent( b, "click", function() {
+ var next = b.parentNode.lastChild,
+ collapsed = hasClass( next, "qunit-collapsed" );
+ ( collapsed ? removeClass : addClass )( next, "qunit-collapsed" );
+ });
+
+ addEvent( b, "dblclick", function( e ) {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

We could just remove this. The rerun link has been there long enough.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

I removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ addEvent( b, "dblclick", function( e ) {
+ var target = e && e.target ? e.target : window.event.srcElement;
+ if ( target.nodeName.toLowerCase() === "span" || target.nodeName.toLowerCase() === "b" ) {
+ target = target.parentNode;
+ }
+ if ( window.location && target.nodeName.toLowerCase() === "strong" ) {
+ window.location = QUnit.url( { testNumber: details.testNumber } );
+ }
+ });
+
+ // `time` initialized at top of scope
+ time = document.createElement( "span" );
+ time.className = "runtime";
+ time.innerHTML = details.runtime + " ms";
+
+ // `li` initialized at top of scope
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

This doesn't even use the right var.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ target = target.parentNode;
+ }
+ if ( window.location && target.nodeName.toLowerCase() === "strong" ) {
+ window.location = QUnit.url( { testNumber: details.testNumber } );
+ }
+ });
+
+ // `time` initialized at top of scope
+ time = document.createElement( "span" );
+ time.className = "runtime";
+ time.innerHTML = details.runtime + " ms";
+
+ // `li` initialized at top of scope
+ testItem.className = bad ? "fail" : "pass";
+ testItem.removeChild( testItem.firstChild );
+ a = testItem.firstChild;
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

Here a comment would be helpful - what's testItem.firstChild at this point? Or above, where it gets removed.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

It was unnecessary, so I did a small refactoring there in @cb09838.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 3, 2014
src/reporter-html.js
+ });
+
+ // `time` initialized at top of scope
+ time = document.createElement( "span" );
+ time.className = "runtime";
+ time.innerHTML = details.runtime + " ms";
+
+ // `li` initialized at top of scope
+ testItem.className = bad ? "fail" : "pass";
+ testItem.removeChild( testItem.firstChild );
+ a = testItem.firstChild;
+ testItem.appendChild( b );
+ testItem.appendChild( a );
+ testItem.appendChild( time );
+
+ // Element repositioning for backwards compatibility
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 3, 2014

I don't understand this. What does this address?

@leobalter
jQuery Foundation member
leobalter added a note Jul 3, 2014

I'll change this to insert b, a and time before assertList.

@leobalter
jQuery Foundation member
leobalter added a note Jul 4, 2014

I did it better in @cb09838.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
jQuery Foundation member

Okay, I'm done. I know that a lot of the code I commented on was just moved. But since its a new file, we should use the opportunity to improve it. Most fixes should be fairly trivial. Refactoring htmlReporterLoad might be too much, that could wait, as long as we have a follow-up ticket.

@leobalter
jQuery Foundation member

Thank you!

Refactoring htmlReporterLoad might be too much, that could wait, as long as we have a follow-up ticket.

Agreed.

Edit: I've split this method on several small functions. It's looking good now.

@leobalter
jQuery Foundation member

I then merged the "async-future" branch (#583) and checked the output in the browser. The assertions that would otherwise leak or disappear now show up in their respective tests. The only concern I have there that the first test now fails with expect saying the it expected one assertion, but zero ran, then afterwards the passed ok assertion shows up. That should probably fail in some way, since it runs after the test is already done.

I understand that test should fail, because it was intentionally - probably - malformed. That's a "should be sync" test that's running an async code.

Also, we can address this in another issue to improve the reporter and generate a warning only, not the expected error.

@leobalter
jQuery Foundation member

Ok, I'm sorry for so many "done" comments, but I needed to check ALL review comments to see if there was something else remaining.

All done now.

The addition of the testNumber property is an API change that we need to document, though I don't see any backwards compatibility concerns with that.

Now that's probably the only follow up issue we need to register.

@jzaefferer jzaefferer commented on an outdated diff Jul 15, 2014
src/reporter-html.js
+ addClass( testList, "hidepass" );
+ }
+
+ return filter;
+}
+
+function toolbarLabel() {
+ var label = document.createElement( "label" );
+ label.setAttribute( "for", "qunit-filter-pass" );
+ label.setAttribute( "title", "Only show tests and assertions that fail. Stored in sessionStorage." );
+ label.innerHTML = "Hide passed tests";
+
+ return label;
+}
+
+function mountToolbar() {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 15, 2014

How about "appendToolbar"? More consistent with existing vocabulary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 15, 2014
src/reporter-html.js
+ var moduleFilter,
+ toolbar = id( "qunit-testrunner-toolbar" );
+
+ if ( toolbar ) {
+ toolbar.appendChild( toolbarFilter() );
+ toolbar.appendChild( toolbarLabel() );
+ toolbar.appendChild( toolbarUrlConfigContainer() );
+
+ moduleFilter = toolbarModuleFilter();
+ if ( moduleFilter ) {
+ toolbar.appendChild( moduleFilter );
+ }
+ }
+}
+
+function mountBanner() {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 15, 2014

Same as above and all following.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff Jul 15, 2014
src/reporter-html.js
+
+ if ( result ) {
+ result.parentNode.removeChild( result );
+ }
+
+ if ( tests ) {
+ tests.innerHTML = "";
+ result = document.createElement( "p" );
+ result.id = "qunit-testresult";
+ result.className = "result";
+ tests.parentNode.insertBefore( result, tests );
+ result.innerHTML = "Running...<br>&nbsp;";
+ }
+}
+
+function resetFixture() {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 15, 2014

The name is misleading, since the resetting happens elsewhere for each test, while this runs just once. How about °storeFixture"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 15, 2014
src/reporter-html.js
+ }
+
+ if ( bad === 0 ) {
+ addClass( assertList, "qunit-collapsed" );
+ }
+
+ // testItem.firstChild is the test name
+ testTitle = testItem.firstChild;
+ testTitle.innerHTML += " <b class='counts'>(<b class='failed'>" + bad +
+ "</b>, <b class='passed'>" + good +
+ "</b>, " + details.assertions.length + ")</b>";
+
+ addEvent( testTitle, "click", function() {
+ var collapsed = hasClass( assertList, "qunit-collapsed" );
+
+ ( collapsed ? removeClass : addClass )( assertList, "qunit-collapsed" );
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 15, 2014

I don't like the use of the ternary here. It avoids duplication, but is rather difficult to parse, for me anyway. The two options I can think of are adding a toggleClass method and using the more verbose if/else. I don't like those either.

@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 15, 2014

This event handler is the only place where we use hasClass directly, the other one is in addClass. Maybe add a toggleClass method anyway and replace the two lines here?

@leobalter
jQuery Foundation member
leobalter added a note Jul 15, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer and 1 other commented on an outdated diff Jul 15, 2014
src/reporter-html.js
+
+function hasClass( elem, name ) {
+ return ( " " + elem.className + " " ).indexOf( " " + name + " " ) >= 0;
+}
+
+function addClass( elem, name ) {
+ if ( !hasClass( elem, name ) ) {
+ elem.className += ( elem.className ? " " : "" ) + name;
+ }
+}
+
+function removeClass( elem, name ) {
+ var set = " " + elem.className + " ";
+
+ // Class name may appear multiple times
+ while ( set.indexOf( " " + name + " " ) > -1 ) {
@jzaefferer
jQuery Foundation member
jzaefferer added a note Jul 15, 2014

I commented on indexOf usage before. This is using > -1, above its >= 0. Let's make those consistent, unless those are in fact not equivalent.

@leobalter
jQuery Foundation member
leobalter added a note Jul 15, 2014

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
jQuery Foundation member

The refactor of htmlReporterLoad makes this much better. There's a few small issues left that I commented on above. I currently can't do any testing though. The steps I outlined in my first comment should be a good reference for someone else to do that though. @JamesMGreene can you do some testing after Leo addressed the remaining issues? Thanks.

@leobalter
jQuery Foundation member

All the remaining issues were addressed.

Thank you for reviewing, this is going to be great.

@Krinkle Krinkle changed the title from Detach the HTML reporter to Decouple HTML reporter from core Jul 15, 2014
@Krinkle
jQuery Foundation member
Krinkle commented Jul 15, 2014

When this gets squashed into fewer commits, it's important we document in the commit message (and eventually in the release notes) at least the following two things:

  • Undocumented internal methods QUnit.id, QUnit.addEvent, QUnit.addClass, QUnit.hasClass, and QUnit.removeClass were removed.
  • The undocumented feature that re-opened the test suite for the html reporter, when adding more assertions after test suite has finished running tests, was removed. This only worked in the html reporter. The callback interface never supported this properly and reporters such as node-qunit, grunt-contrib-qunit and TestSwam never supported this either. If this causes trouble, be sure to verify QUnit does not start until after your tests are loaded (maybe mention config.autostart).
@leobalter
jQuery Foundation member

Thank you @Krinkle.

I started squashing but kept the single commits in another branch.

@leobalter leobalter Core: Improve logging callbacks
- Remove deprecated support for overwriting logging callbacks
- Remove scope param from runLoggingCallbacks
31cad90
@leobalter leobalter added a commit to leobalter/qunit that referenced this pull request Jul 15, 2014
@leobalter leobalter Reporter: Move HTML reporter to a new JS file
- Removes QUnit.init and deprecated QUnit.reset
- Extends QUnit.extend to accept a `undefOnly` parameter,
  so we can check if it's needed to extend only unset/undefined properties
- HTML Logging is now related to each assertion through QUnit.log
- Now it logs exactly on the assertion's related test, without leaking. Ref #583
- The HTML reporter was refactored and improved. Ref #603
2a77743
@leobalter
jQuery Foundation member

@Krinkle: almost all the commits have a long description now, and specially @5184ccf and @c4b49a3 have the solicited comments.

@leobalter
jQuery Foundation member

I also did the squash in a way I consider it's good to merge.

@leobalter
jQuery Foundation member

Let's ship it?

@JamesMGreene
jQuery Foundation member

LGTM. :shipit:

Do we anticipate include more reporters in QUnit core? If so, I would suggest moving src/reporter-html.js into a "reporters" folder, i.e. src/reporters/html.js. If we're unsure, we can make that call later.

leobalter added some commits Jun 24, 2014
@leobalter leobalter Reporter: Move HTML reporter to a new JS file
- Removes QUnit.init and deprecated QUnit.reset
- Extends QUnit.extend to accept a `undefOnly` parameter,
  so we can check if it's needed to extend only unset/undefined properties
- HTML Logging is now related to each assertion through QUnit.log
- Now it logs exactly on the assertion's related test, without leaking. Ref #583
- The HTML reporter was refactored and improved. Ref #603
113a9d5
@leobalter leobalter Core: Remove QUnit.* undocumented reporter methods
Undocumented internal methods QUnit.id, QUnit.addEvent, QUnit.addClass,
QUnit.hasClass, and QUnit.removeClass were removed.
2000ed3
@leobalter leobalter Core: Register logging callbacks on config.callbacks 7a23fd8
@leobalter leobalter Tests: Remove after-done tests
The undocumented feature that re-opened the test suite for the html reporter,
when adding more assertions after test suite has finished running tests,
was removed. This only worked in the html reporter.

The callback interface never supported this properly and reporters such as
node-qunit, grunt-contrib-qunit and TestSwam never supported this either.

If this causes trouble, be sure to verify QUnit does not start until after
your tests are loaded (maybe using config.autostart).
36020d9
@leobalter leobalter Build: Detach jsDiff and HTML Reporter from QUnit
They're still built together anyway, but this allows us to make a special
build for QUnit to report on console only
40f427b
@leobalter
jQuery Foundation member

@JamesMGreene it's done and passing through tests, also checked on browserstack.

@coveralls

Coverage Status

Coverage increased (+2.4%) when pulling 40f427b on leobalter:600-assert-start-stop into fef0e59 on jquery:master.

@JamesMGreene JamesMGreene merged commit 40f427b into jquery:master Jul 22, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@JamesMGreene
jQuery Foundation member

Merged. Thanks!

@leobalter
jQuery Foundation member

\o/ Thank you!

@leobalter leobalter deleted the leobalter:600-assert-start-stop branch Jul 22, 2014
@JamesMGreene JamesMGreene self-assigned this Jul 22, 2014
@JamesMGreene JamesMGreene added this to the pre-2.0 milestone Jul 22, 2014
This was referenced Jul 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.