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.
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
feat: Allow mocking property value in tests #13496
feat: Allow mocking property value in tests #13496
Changes from all commits
15697c7
9c270ca
20b881b
930036d
c739864
0f227c2
1c4535b
4a1aeb6
c79837d
cceffa0
6c17cc0
d828f11
4f9ac47
b3fb383
208df4d
2ed2ca8
ba36a3b
25d1b24
472841c
2f9c9c9
33c30b9
f0ffae1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
question: Why do we replace all of
process.env
instead of replacing only the value we're interested in i.e. why notinstead?
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.
yeah, that seems better, although then
replacedEnv
seems wrong?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.
This opens a design question. One cannot right now replace property that does not exist on an object. Assuming 'HOSTNAME' might or might not be in the env... What should we do in this case?
The Sinon.js implementation forbids creation of new properties. It must exist on an object (or in the prototype chain). I applied same restriction here.
What do you think, @SimenB , @mrazauskas and @eps1lon ? Should we allow adding undefined properties to the object (which is actually quite simple to do), or should be more restrictive (and possibly open this behavior in future). I do not have strong thoughts here...
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.
Hm good point. I would expect that we can mock non-existing properties. But can see how
replaceProperty
might be misleading. But again, from my intuition, we should be able to mock non-existing properties.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.
we could have a separate options bag (
{mustExist: true}
by default or something) that allows customization? But I think by default we should not allow it - if nothing else to catch typos (TS might help of course).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.
Yeah that works for our use case. And the
process.env
use case probably also works with aprocess.env.HOST_NAME = undefined
during setup since I haven't seen any use case where you differentiate between unknown property andundefined
inprocess.env
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.
@eps1lon: I am a bit afraid of typos or invalid usages. People might be mocking stuff:
And if one thinks about it... How common/rare is the use case when we need to mock something that does not exist? I can think of a case when people might want to mock either a new key of "dictionary" or some new array element. In that case, they should mock the parent anyway I guess... In same manner like the process.env. E.g.:
@SimenB This makes sense. How should I proceed?
mustExist: true
/allowUndefined: false
/allowNonexistent: false
/ ...? I am fine withmustExist
)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.
We can do that in a followup to allow mocking non-existing props. But if you wanna get started right away that'd be awesome 😀
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.
Thanks, this makes sense. I will implement it as separate PR, as this is already growing quite a lot. :)