Core: Exclude or grey internal frames from stack traces in TAP reporter #1789
Conversation
This file contains 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
Internal frames are those from qunit.js, or Node.js runtime. * Remove any internal frames from the top of the stack. * Grey out later internal frames anywhere in the stack. This change is applied to the TAP reporter, which the QUnit CLI uses by default.
Krinkle
force-pushed
the
clean-cli-err-stack
branch
from
bd67758 to
228c073
Compare
Krinkle
added a commit
to Krinkle/qunit
that referenced
this pull request
Jul 27, 2024
* Remove hacky re-entrance from ProcessingQueue.done() -> test() + advance() -> done(), existed only for this purpose. This also removes the need for the 99aee51 workaround, which avoided a crash by infinite loop. * Remove unused internal `test` injection to ProcessingQueue, existed only for this purpose. * Remove "omit stack trace" logic in test.js, existed only for this purpose. To keep output for the "No tests" error similarly clean and distraction-free, the TAP reporter treats error stack traces with a similar cleaner since qunitjs#1789. * Remove unused internal `validTest` mechanism existed only for this purpose. This was originally impossible to trigger externally because it required setting `validTest` to a private symbol. In QUnit 1.16 this was simplified as part of commit 3f08a1a, to take any boolean true value to ease some implementation details, however it remained internal in purpose. A search today for `/validTest:/` and `/validTest = /` over public GitHub-hosted repositories, shows that (fortunatley) nobody has started relying on this. I found only copies of QUnit itself. As a nice side-effect, fixtures like async-module-error.tap.txt now no longer display a useless "No tests" after an uncaught error that already bailed the test run. This happened previously because errors are not tests, and so technically there was no test. No that "No tests" is itself considered a bail out, TAP absorbs this after the first error, just like it alreayd does for other cascading errors.
Krinkle
added a commit
that referenced
this pull request
Jul 27, 2024
* Remove hacky re-entrance from ProcessingQueue.done() -> test() + advance() -> done(), existed only for this purpose. This also removes the need for the 99aee51 workaround, which avoided a crash by infinite loop. * Remove unused internal `test` injection to ProcessingQueue, existed only for this purpose. * Remove "omit stack trace" logic in test.js, existed only for this purpose. To keep output for the "No tests" error similarly clean and distraction-free, the TAP reporter treats error stack traces with a similar cleaner since #1789. * Remove unused internal `validTest` mechanism existed only for this purpose. This was originally impossible to trigger externally because it required setting `validTest` to a private symbol. In QUnit 1.16 this was simplified as part of commit 3f08a1a, to take any boolean true value to ease some implementation details, however it remained internal in purpose. A search today for `/validTest:/` and `/validTest = /` over public GitHub-hosted repositories, shows that (fortunatley) nobody has started relying on this. I found only copies of QUnit itself. As a nice side-effect, fixtures like async-module-error.tap.txt now no longer display a useless "No tests" after an uncaught error that already bailed the test run. This happened previously because errors are not tests, and so technically there was no test. No that "No tests" is itself considered a bail out, TAP absorbs this after the first error, just like it alreayd does for other cascading errors. Closes #1790.
Krinkle
added a commit
to Krinkle/qunit
that referenced
this pull request
Aug 7, 2024
Follows-up qunitjs#1789, which applied this to traces under uncaught errors. We now apply the same to traces under assertion failures as well. (TODO: Split into separate commit) Omit noisy and confusing actual:null,expected:undefined from TAP output. This is prevented in HtmlReporter due to hasOwn check, but this check fails in TAP because it uses testEnd.errors instead of QUnit.log(). And, in Test.js#logAssertion, where originally expected is allowed to be unset, it gets forged in the call to testReport in order to satisfy the API. Indeed, js-reporters never allowed for unset actual/expected. Fix this by instead deciding to skip rendering if both are strictly equal to the value of undefined, which I believe cannot be useful information. However, for that to be true, we have to change `actual:null` to `actual:undefined` in pushFailure(). Otherwise, it can easily be a genuine diff.
Krinkle
added a commit
that referenced
this pull request
Aug 7, 2024
Follows-up #1789, which applied this to traces under uncaught errors. We now apply this clean up to traces under assertion failures as well.
Krinkle
added a commit
that referenced
this pull request
Jan 18, 2025
…port) Cherry-picked from 9fed286 (3.0.0-dev): > Core: Exclude or grey internal frames from stack traces in TAP reporter > Internal frames are those from qunit.js, or Node.js runtime. > > * Remove any internal frames from the top of the stack. > * Grey out later internal frames anywhere in the stack. > > This change is applied to the TAP reporter, which the QUnit CLI uses > by default. > > Ref #1789. Cherry-picked from 95105aa (3.0.0-dev): > Fix fragile code in stracktrace.js that previously worked only because > of Babel transformations masking a violation of the Temporal Dead Zone > between `const fileName` and the functions it uses to compute that > value.
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.
Internal frames are those from qunit.js, or Node.js runtime.
This change is applied to the TAP reporter, which the QUnit CLI uses by default. The benefit of trimming initial qunit.js internal frames also benefits use of the TAP reporter in browser environments.
This change inspired by an upcoming change, which converts our fake "No tests" test failure into a normal error. When we emitted it as a fake test, it benefitted from QUnit.stack() trimming qunit.js frames, thus guanteeing the stack to start at the QUnit.test() call, skipping the first few frames, and anything after the user code is also by definition not of interest.
For generic errors, we weren't doing this yet. And, while we don't have a clean anchor point like QUnit.test() for uncaught errors, we can do better than dumping it all to the console. We can still safely trim the first frames that are internal. Removing later ones is likely to cause confusion (by wrongly suggesting that
A>Cwhen it is actuallyA>B>C). We can color those in grey, similar to what Node.js's own error formatter does by default.Example
Using
node bin/qunit.js test/cli/fixtures/async-module-error.jsas example. The stack trace now starts at the test suite file, skipping qunit.js itself as first two frames. This is similar to what the HTML Reporter has done for years for stack traces under assertions (rather than global errors).Before (3.0.0-alpha.1)
After (3.0.0-alpha.3)
