test_runner: fix #50665 .skip, .todo and .only missing in subtests#50673
test_runner: fix #50665 .skip, .todo and .only missing in subtests#50673rluvaton wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom rluvaton:fix-50665-only-skip-todo-not-available-for-subtestrluvaton/node:fix-50665-only-skip-todo-not-available-for-subtestCopy head branch name to clipboard
.skip, .todo and .only missing in subtests#50673Conversation
|
Review requested:
|
638b7b6 to
63fdacf
Compare
63fdacf to
8834f4b
Compare
| this.test = (name, options, fn) => { | ||
| const overrides = { | ||
| __proto__: null, | ||
| loc: getCallerLocation(), | ||
| }; | ||
| // eslint-disable-next-line no-use-before-define | ||
| const subtest = this.#test.createSubtest(Test, name, options, fn, overrides); | ||
| return subtest.start(); | ||
| }; |
There was a problem hiding this comment.
wouldn't it be more performant to leave this as a prototype method, and instead do a public class field test = FunctionPrototypeBind(this.test, this);, so that the "meat" of the method can be shared and optimized across all instances?
separately, couldn't the skip/todo/only methods be assigned to the prototype method once, in a static block, instead of a new function created on every instance?
There was a problem hiding this comment.
If you mean with Object.defineProperty, then without changing the #test to [kTest] it can't as it cannot access private method outside the class.
if you mean adding the skip/todo/any in the constructor but keeping the test function as is, than for some reason it did not work for me
The skip/todo/any methods are needed to assign on the test function, not on the TestContext
There was a problem hiding this comment.
Ah, true, the 3 expandos need to be done in the constructor every time - altho the meat could be shared private methods and the per-instance thing could just be a bound function.
| this.#test.todo(message); | ||
| } | ||
|
|
||
| test(name, options, fn) { |
There was a problem hiding this comment.
why not keep as prototype methods?
There was a problem hiding this comment.
for some reason it did not work for me adding skip/todo/etc to the test function
|
The docs for
How is that supposed to map to use of
Presumably this implements the first one (though I don't see any tests for that scenario). Either way the docs should be updated, because right now they imply that
|
|
Removed the
author ready
|
8834f4b to
8ba986d
Compare
… subtests
8ba986d to
77128c7
Compare
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
Fix #50665