HTML Reporter: Add support for displaying early errors#1786
Merged
Conversation
712b5e5 to
5f6821d
Compare
5f6821d to
722acf2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Memory for 'error' event
The status quo, in both QUnit 1.x and 2.x (2.21 as of writing) is that "early" errors are only shown in the console and not shown in the UI by the HTML Reporter. This includes uncaught errors while your subject source code is being loaded/defined, and any global errors from any test filed before the first test begins.
In QUnit 2.21 today, we initialise the HTML Reporter as part of the qunit.js execution, which means we are listening to all events right away. That sounds good, except it is kind of "too early" in a way, because the
onErrorhandler doesn't know what to do with it, as it can't render it in the UI yet, because we don't create the UI until the DOM is ready.My initial approach to fixing this for QUnit 3, was to add a little buffer inside HtmlReporter where, the early return in
onErrorwould stash theerrorin athis.earlyErrorfield. Then, inonBegin(), if we have anythis.earlyError, pass it throughonErrora second time.onBegin (beginDetails) { this.appendInterface(beginDetails); this.elementDisplay.className = 'running'; + + if (this.earlyError) { + this.onError(this.earlyError); + this.earlyError = null; + } }onError (error) { const testItem = this.elementTests && this.appendTest('global failure'); if (!testItem) { // … + this.earlyError = error; return; }This approach worked when I first started working on this patch a few weeks ago. However, commit 05e15ba moved the initialising of reporters (incl. HTML Reporter) to happen later, inside
QUnit.start(), just in time before emittingrunStartand invokingonBegin.That's a problem, because it means that an
errorevent that we previously received (but didn't do anything with), is now not being received at all, because we're not starting to listen for it until after it's already happened. Even the newprioritizeSignaldoesn't help us here, because that only re-orders the event handlers for future events, it doesn't bring back the past.We could work around this by reverting the HTML Reporter to unconditionally init early, and double-checking whether it's supposed to be enabled or not inside HtmlReporter.onRunStart. But.. that feels like a hack, and doesn't solve it for the other reporters.
Instead, this pull request applies the new
MEMORY_EVENTSmechanism toerror. That seems worthwhile regardless, for example, I noticed in grunt-contrib-qunit recently that it has a workaround that listens both for puppeteerpagerrorandQUnit.on('error'), because Puppeteer's injected JS either runs before qunit.js is defined, or waits for DOMContentLoaded. That makes sense because on the consumer side, there is no good middle-ground there. Enabling "memory" for this QUnit event, would mean such workarounds will no longer be needed, thus making QUnit events more reliable.inArray bug
This patch fixes a mistake in
events.jsthat's been there since commit 8f25f26. It passed arguments toinArray()in the wrong order. This code is tested and was passing due to a lucky set of mistakes cancelling each other out.In addition to Array.includes, there is also String.includes. This casts any argument to a string. So when called as
string.includes(array), the array is implicitly coerced to a string. The default Array.toString in JavaScript returnsArray.join(', '). For a single-value array, that is equivalent to the value, because['runEnd'].toString() === 'runEnd', and'runEnd'.includes('runEnd') === true, naturally. However,'runEnd'.includes('error,runEnd')is not.