-
Notifications
You must be signed in to change notification settings - Fork 783
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
v2.16.0+ reports wrong source in browser #1679
Comments
Thank you for the clear examples, that helps a lot. I've confirmed the bug. To clarify, this is about the source attribution for passing tests, indicating where the test block starts. It is not about the stack trace for failing tests, which seems to be unaffected. I'll also note that while this bug appears consistent on your demo, I do note that in other cirumstances the source is still correctly reported. For example, running QUnit's own tests, it reports:
Which shows both URLs. The former of which isn't useful and that's a regression there as well, but might explain why it wasn't noticed earlier. Sorry about that. I'll get this fixed right away. |
That was a fun little investigation. This is mostly for myself, but I'll post it here regardless. Original stack traceUpon defining a test case via
Note that the stack trace simply starts at the bottom with your test suite, and then enters QUnit internals. Those higher frames are not of interest to the developer. If you use nested modules, then the stack might go a bit further:
Users also sometimes use utility function for defining multiple tests (although we now have QUnit.test.each):
Lastly, when testing asynchronous code, modern browsers will provide the details of where the first execution started, which can be the browser The point is: Only a specific specific range in the trace is relevant. We want to skip the top frames that are internal. Then keep the ones from your code. And then then skip everything else, even if it goes back to your code, since that will be previous unrelated code now. The way it workedWhen the user interface asks for Source line, we compute it by analyzing the stacktrace. The default HTML Reporter does this. The QUnit CLI, TAP Reporter, and most cloud/CI integrations do not do this, as they tend to only report failed tests or provide only the name of the test for progress reporting. The analysis is quite simple (source: src/core/stracktrace.js):
The problem is of course that the code immediately thought that there were 0 frames of your code, and then the third frame is the "start" of more internal frames. |
Follows-up commits 07de4b1 and 835b7c1. This introduced `test.each()`, but also changed `QUnit.test()` to have 3 instead of 2 function calls until `new Test()` internally, which broke the stacktrace extraction in most cases. This was not covered well by tests. The `test.each()` itself also had a different offset. It had 6 frames until `new Test` for the case of array data, and 7 frames for the case of object data. This difference is due to `data.forEach(fn)` verses `keys.forEach(x => fn())`. * Cover it all with tests. * Increase default stackOffset from 2 to 3. * Change `runEach` to always have 5 frames, by using a for-loop. * Allow internal override. * Use the override. Fixes #1679.
Tell us about your runtime:
2.16.0 (and after) can also reproduce the bug with 2.18.0
browser
manually in browser
What are you trying to do?
Code that reproduces the problem: https://github.com/phanirithvij/tarballjs check the
tests/
folderI've added tests for both 2.15.0 and 2.16.0 in the
tests/
folder in the repo I've provided.They are accessible via github pages here.
Regression in 2.15.0...2.16.0
What did you expect to happen?
Sources should be reported correctly (works in 2.15.0)
Screenshot
What actually happened?
From version 2.16.0 onwards the source file is being reported as
Screenshot
The text was updated successfully, but these errors were encountered: