feat: Allow mocking property value in tests

[michal-kocarek]

Summary

PR is adding new feature that allows mocking values of object properties, that are not methods. Method is inspired to Sinon.js'es sandbox.replace(object, property, replacement);, but written from scratch.

Ref: https://sinonjs.org/releases/v14/sandbox/#sandboxreplaceobject-property-replacement

Method is now named replaceProperty(), is sitting next to the spyOn() method in jest-mock package. Ideally, it should be exposed in global Jest object as well.

replaceProperty() returns an object that exposes replaceValue() method and restore() methods. The restorer is called also in restoreAllMocks() cleanup method.

I tried to limit the usage as much as possible, so it could be potentially extended without breaking any compatibility. That is why it:

1. forbids replacement of functions (one should use spyOn() for that)
2. forbids replacement of getters and setters (just for sake of simplicity)

Fixes #13465.

The motivation for this feature is to allow simpler way of mocking property values, which, right now, have to be split into multiple places, which might introduce problems on multiple places:

Before:

let oldEnv;

beforeEach(() => {
  oldEnv = process.env;
  process.env = {
    FOO: 1,
  };
});

afterEach(() => {
  process.env = oldEnv;
});

After:

beforeEach(() => {
  jest.replaceProperty(process, 'env', {FOO: 1});
});

afterEach(() => {
  jest.restoreAllMocks();
});

After (advanced usage):

let processEnvMock;

beforeEach(() => {
  processEnvMock = jest.replaceProperty(process, 'env', {FOO: 1});
});

test('additional mock', () => {
  processEnvMock.replaceValue({FOO: 1, BAR: 2});
});

afterEach(() => {
  processEnvMock.restore();
});

Test plan

• Unit tests have been added.
• Examples have been added.
• Type tests have been added.
• E2E tests have been added.

[mrazauskas]

Looks very useful!

I think the shape of the returned value should be somewhat similar to MockInstance type. mockRestore is already there, why not to have: mockPropertyValue, mockPropertyValueOnce? Also mock prop with access data sounds useful. The shape could be similar to MockFunctionState (can be called MockPropertyState), for instance it could count how many times the property was accessed.

I think jest.mockProperty is good name. Could be also jest.spyOnProperty. Or even:

jest.spyOn(process, 'stdout').mockPropertyValue({ isTTY: true });

In a way that would make sense if accesses counted.

---

forbids replacement of property that has been already replaced

I like to keep it strict, but it might be useful to see what spyOn does in similar situation right now? Feeling says that behaviour should be somewhat similar in both cases.

---

EDIT: Just read your #13465 (comment). I get your point, spying might not work here.

[mrazauskas]

On second though, jest.replaceProperty might be better name. Jest mocks as well as spies have state or the mock prop. In this case it does not make much sense to keep any state. So I think mock or spy should not appear in the name to clearly show that this is just a replacement mechanism and there is no mock.

Just like jest.useFakeTimers is not jest.mockTime. That is clearly not a mock.

---

Calling the method on a replaced property could simply change the value. Perhaps? To allow having different value for each test. Somewhat like calling jest.useFakeTimers reinstalls the fake timers using different options.

[michal-kocarek]

So what about an interface like this:

// Used to be jest.mockProperty()
const replacer = jest.replaceProperty(obj, 'prop', value);

// Used to be replacer.mockRestore()
replacer.restore();

---

I like to keep it strict, but it might be useful to see what spyOn does in similar situation right now? Feeling says that behaviour should be somewhat similar in both cases.

Also, let's add support for calling replaceProperty() mutliple times on same property without throwing. It will be same as calling jest.spyOn() multiple times on same method.

jest.replaceProperty(process, 'env', {});

// Used to throw, will be working now
jest.replaceProperty(process, 'env', {FOO: 1});

---

Do you think this could make better sense?

[mrazauskas]

I like it this way.

The method could also return Jest Object for chaining, but I prefer having replacer.restore(). (It will be also called by jest.restoreAllMocks(), right?)

Just a detail, it would make sense to have jest.Replaced<Source> type util:

let replacedStdout: jest.Replaced<typeof process.stdout>;

beforeEach(() => {
  replacedStdout = jest.replaceProperty(process, 'stdout', { isTTY: true });
});

afterEach(() => {
  replacedStdout.restore();
});

---

@SimenB @cpojer what you think about the API?

[michal-kocarek]

Hi @mrazauskas yes, adding jest.Replaced makes sense.

Should I progress in finalizing this PR?

[mrazauskas]

It might be better to wait for response from the maintainers before moving forward.

[michal-kocarek]

Comments are fixed, let me know what you think, @eps1lon, @SimenB.
Thank you.

[mrazauskas]

Thanks. I left few type related comments.

What about documentation and jest.MockedProperty utility type? Reference and inspiration: #13440

[eps1lon]

@SimenB Can we get a CI run? Is this going in the right direction or are there any potential blockers you can see?

[michal-kocarek]

Thank you for valuable review.

I've tried to fix what I could, changed the method names as per @mrazauskas request and added some documentation. Also, there is now a utility type jest.Replaced with necessary documentation and examples.

I will be looking forward for your another round of thoughts...

[michal-kocarek]

Thanks! It looks better and better. Few more ideas from my side.

Thank you for detailed review, @mrazauskas.
PR is updated with your requests and for the unresolved I will be looking for your response.

[eps1lon]

question: Why do we replace all of process.env instead of replacing only the value we're interested in i.e. why not

Suggested change

	replacedEnv = jest.replaceProperty(process, 'env', {HOSTNAME: 'localhost'});
	replacedEnv = jest.replaceProperty(process.env, 'HOSTNAME', 'localhost');

instead?

[SimenB]

yeah, that seems better, although then replacedEnv seems wrong?

[michal-kocarek]

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...

[eps1lon]

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?

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.

[SimenB]

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).

[eps1lon]

Yeah that works for our use case. And the process.env use case probably also works with a process.env.HOST_NAME = undefined during setup since I haven't seen any use case where you differentiate between unknown property and undefined in process.env

[michal-kocarek]

But again, from my intuition, we should be able to mock non-existing properties.

@eps1lon: I am a bit afraid of typos or invalid usages. People might be mocking stuff:

• that is not used due to typo,
• that exists now, but after upgrade of a library disappears (e.g. mocking private handle of some database client instance),
• due to error - misinterpreting API documentation and trying to mock property for different class.

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.:

// Instead of
jest.replaceProperty(httpAgent.defaultHeaders, 'X-API-Key', mockKey);

// Use
jest.replaceProperty(httpAgent, 'defaultHeaders', {...httpAgent.defaultHeaders, 'X-API-Key': mockKey});

• ❓ If we keep current behavior, should I mention in documentation how to work with undefined propeties - e.g. in process.env? This might be quite common case.

---

...separate options bag ({mustExist: true} by default... by default we should not allow it - if nothing else to catch typos (TS might help of course).

@SimenB This makes sense. How should I proceed?

• ❓ Should I implement the optional ability to replace undeclared properties?
• If yes, what is best name for the option inside property bag? (mustExist: true / allowUndefined: false / allowNonexistent: false / ...? I am fine with mustExist)

[SimenB]

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 😀

[michal-kocarek]

Thanks, this makes sense. I will implement it as separate PR, as this is already growing quite a lot. :)

[SimenB]

This is looking great! Thank you so much!!

[SimenB]

ergh, actually CI is failing 😅

Code looks good (great, even 😀) to me, though 👍

[mrazauskas]

@michal-kocarek Would you be up to implementing allowUndefined (or maybe tolerateUndefined) option?

I wanted to use jest.replaceProperty in Jest repo. Very first attempt threw "BABEL_ENV property does not exist":

https://github.com/facebook/jest/blob/836157f4807893bb23a4758a60998fbd61cb184c/packages/jest-create-cache-key-function/src/__tests__/index.test.ts#L10-L23

[vostafi-impressit]

Looks like replaceProperty is not available on global object jest
Could anyone check it?

[SimenB]

Looks like replaceProperty is not available on global object jest Could anyone check it?

Please open up a new issue with a reproduction

[mrazauskas]

Just opened a PR for tolerateUndefined option (#13958). It was discussed here, feel free to leave feedback (;

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.