-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
test_runner: add support for chainable mockImplementationOnce
#49088
Changes from all commits
4bb7656
fdf1e08
b3607e1
3488751
478f256
3aa353a
b03cf4e
6a41fe8
8490dcb
56512d2
3965392
9eb89ee
92f225f
ff0c74d
84e98f3
a6e98fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
rluvaton marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,9 +61,24 @@ class MockFunctionContext { | |
mockImplementationOnce(implementation, onCall) { | ||
validateFunction(implementation, 'implementation'); | ||
const nextCall = this.#calls.length; | ||
const call = onCall ?? nextCall; | ||
validateInteger(call, 'onCall', nextCall); | ||
this.#mocks.set(call, implementation); | ||
// eslint-disable-next-line eqeqeq | ||
if (onCall != undefined) { | ||
validateInteger(onCall, 'onCall', nextCall); | ||
this.#mocks.set(onCall, implementation); | ||
|
||
return this; | ||
} | ||
|
||
const mocksSize = this.#mocks.size; | ||
for (let i = 0; i < mocksSize; i++) { | ||
if (!this.#mocks.has(i + nextCall)) { | ||
this.#mocks.set(i + nextCall, implementation); | ||
return this; | ||
} | ||
} | ||
Comment on lines
+72
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this is right either (although I haven't tested this variation). I think you want a counter, and you want to loop over all of the existing entries in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This solution find the first empty implementation when you go incrementally, and then put it there I don't see a problem with this and I would appreciate if you have a concrete example 😀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change: test('foo', (t) => {
let cnt = 0;
function addOne() {
cnt++;
return cnt;
}
const fn = t.mock.fn(addOne);
fn.mock.mockImplementationOnce(() => 999, 0);
fn.mock.mockImplementationOnce(() => 888, 1);
fn.mock.mockImplementationOnce(() => 777);
console.log(fn());
console.log(fn());
console.log(fn());
}); This currently prints 777, 888, 1. With the changes in this PR, it now prints 999, 888, 777 (note that I'm not even using chaining here). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is a breaking change this is why I wrote that in the PR description, what do you propose? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I didn't notice that you had updated the PR description recently. I'm not sure if chaining can be implemented without a breaking change. I personally prefer the simplicity of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it can be implemented (only adding chaining) but I think it's gonna be unintuitive DX... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I got that it's a breaking change but this behavior doesn't look right. I love the chaining possibility and I believe it's a great addition! In the algorithm, shouldn't we just enqueue all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do "enqueue" it by putting that in the next available location There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
hmm, why is the order reversed on @cjihrig 's example? is it because of the second param? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to show the breaking change |
||
this.#mocks.set(mocksSize + nextCall, implementation); | ||
|
||
return this; | ||
} | ||
|
||
restore() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to emphasize that it append the implementation as it wasn't before, and instead it just replaced it when mocking multiple times before calling the function