Adding debug info to test isolation validation #373
Conversation
| @@ -36,13 +68,23 @@ export function reportIfTestNotIsolated() { | |||
| let leakyTests = TESTS_NOT_ISOLATED.slice(); | |||
| TESTS_NOT_ISOLATED.length = 0; | |||
|
|
|||
| throw new Error(getMessage(leakyTests.length, leakyTests.join('\n'))); | |||
| throw new Error(getMessage(leakyTests.length, leakyTests.join('\n'), _debugInfoSummary)); | |||
There was a problem hiding this comment.
I think we want to console.error the various stacks that are still pending here, that will ensure the source maps are applied and makes things much more actionable IMHO
There was a problem hiding this comment.
So the summary will not contain the stacks. We'll format the stacks separately to output to the console. This is a summary view, which is basically
TESTS ARE NOT ISOLATED
The following (13) tests have one or more of pending timers, pending AJAX requests, pending test waiters, or are still in a runloop:
<list of tests>
We found the following information that may help you identify code that violated test isolation:
Pending timers: 3
Pending scheduled items: 0
More information has been printed to the console. Please use that information to help in debugging.
scalvert
force-pushed
the
add-debug-info
branch
from
9f29aa1
to
a1d0001
Compare
addon-test-support/ember-qunit/-internal/test-debug-info-summary.js
Outdated
Show resolved
Hide resolved
| this._testDebugInfos.push(testDebugInfo); | ||
|
|
||
| this.fullTestNames.push(summary.fullTestName); | ||
| this.hasPendingRequests |= summary.hasPendingRequests; |
There was a problem hiding this comment.
ZOMG my bits are all flipped! (🤡 bits...)
There was a problem hiding this comment.
Seriously though, can we use booleans? My brain can't handle the bitwise operations...
There was a problem hiding this comment.
These are booleans. We're aggregating all the booleans from all the TestDebugInfo instances to determine, overall, if we have any pending requests from any of our tests. Remember, this is a summarized view.
There was a problem hiding this comment.
What I’m saying / asking is to avoid using bitwise operations (which |= is).
There was a problem hiding this comment.
What's a better way to aggregate all the booleans?
| let testDebugInfo = new TestDebugInfo(module, name, getSettledState()); | ||
|
|
||
| if (typeof backburner.getDebugInfo === 'function') { | ||
| testDebugInfo.debugInfo = backburner.getDebugInfo(); |
There was a problem hiding this comment.
In general, I'd prefer to do this above the testDebugInfo creation so that we can pass it into the constructor, what do you think?
yarn.lock
Outdated
| @@ -5,14 +5,12 @@ | |||
| "@babel/code-frame@^7.0.0": | |||
| version "7.0.0" | |||
| resolved "https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0.tgz#06e2ab19bdb535385559aabb5ba59729482800f8" | |||
| integrity sha512-OfC2uemaknXr87bdLUkWog7nYuliM9Ij5HUcajsVcMCpQrcLmtxRbVFTIqmcSkSeYRBFBRxs2FiUqFJDLdiebA== | |||
There was a problem hiding this comment.
Can you update your global yarn version to 1.10.1 so we can avoid removing the integrity field data?
There was a problem hiding this comment.
Clearly I have no integrity...
scalvert
force-pushed
the
add-debug-info
branch
from
eed7547
to
d84bd67
Compare
| TESTS_NOT_ISOLATED.length = 0; | ||
| if (nonIsolatedTests.hasDebugInfo) { | ||
| nonIsolatedTests.printToConsole(); | ||
| nonIsolatedTests.reset(); |
There was a problem hiding this comment.
This is somewhat curious, wouldn't it be easier to do:
nonIsolatedTests = new TestInfoSummary();If we do that, then AFAICT we don't need reset?
|
@rwjblue I think we're good here. Anything else you think I should add? |
|
What about |
|
IMHO, that should be separate |
rwjblue
changed the title
This PR builds on the existing work to detect when tests are not isolated.
Debug info is extracted from backburner using the new
getDebugInfomethod, which allows us to provide information, including stack trace information, to end users. We output that debug info to the console to allow 'click to open' functionality from within the stack traces, and additionally provide a summary of the debug info to test output.TODO:
backburner.DEBUG = truewe gather and output debug info