Add support for tracking the asserts that ran in testharness.js #27224
Conversation
|
There are no reviewers for this pull request besides its author. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you! |
|
I'm looking at options for testing this. |
There was a problem hiding this comment.
Generally I like the idea. I wasn't able to experiment with it locally however as it threw an error on dom/historical.html (see comment).
Have you done any performance profiling? If this is expensive (we have tests with 1000s of subtests, each of which may have many asserts), we may want to make it optional.
jgraham
force-pushed
the
asserts_tracking
branch
from
490bf58
to
ad1089a
Compare
I made it only happen when we have output enabled since it's purely informative at this point and not part of the API. In a test with 17,000 subtests on my machine we spent about 1s extra running this (mostly formatting the arguments), so that's not quite trivial (although it is an extreme case). |
This keeps a global list of all the asserts that are triggered along with details about the argument types and so on. We could make this per test, but the idea is that keeping the list globally provides additional informataion about ordering that might be relevant in some cases. Since this may have some perf impact and is only useful for debugging, and not part of the API, we only collect this data when visual output is enabled. To keep the arguments we are currently formatting as a string. This isn't terribly efficient, but the concern is that keeping the actual object alive will increase memory usage and may affect tests. Also the formatting itself could possibly be improved.
This adds a <details> element under each test message with the asserts that ran, their status, and the line where they were called.
jgraham
force-pushed
the
asserts_tracking
branch
from
ad1089a
to
4b95851
Compare
There was a problem hiding this comment.
I made it only happen when we have output enabled since it's purely informative at this point and not part of the API. In a test with 17,000 subtests on my machine we spent about 1s extra running this (mostly formatting the arguments), so that's not quite trivial (although it is an extreme case).
To me that's pretty trivial (given how extreme that case is); I suspect the extra 1s in that case is a small percent overhead.
I love this PR, so much. It LGTM as is (though see my one architectural comment on whether to associate asserts with a test object proactively), but I would like to see tests before I approve ;)
PS: https://web-platform-tests.org/writing-tests/testharness-api.html?highlight=add_completion_callback#callback-api will need updated
| @@ -3395,6 +3486,51 @@ policies and contribution forms [3]. | |||
| return ''; | |||
| } | |||
|
|
|||
| var asserts_run_by_test = new Map(); | |||
There was a problem hiding this comment.
I wonder if it would be nicer to associate asserts with a Test right from the start? (I.e. instead of pushing them into a separate array). Right now every user of completion callback will have to write code to turn the entire assert array into a test--> assert list map.
The only question might be what to do in set_assert when current_test is null, which here is neatly handled because Map allows that. Of course maybe its a good chance to raise a harness error and be like "y'all missed a step call!"
There was a problem hiding this comment.
I didn't do that because in theory the ordering of asserts between tests could be significant. We don't actually expose that at the moment, but we could. We could store that in some other way of course e.g. by giving each assert an index. idk if you think that's valuable enough to retain the more complex data model.
There was a problem hiding this comment.
This is probably fine then. If we ever decide otherwise there's a fairly easy deprecation path (add to Test as well, then later remove the third argument)
jgraham
force-pushed
the
asserts_tracking
branch
from
4b95851
to
ac8814d
Compare
|
I've added a small amount of testing based on the existing framework. Do you think that approach is good enough? It doesn't really check the output, but does check that we manage to push the relevant stuff to the asserts array. |
No description provided.