remove response delay in node

[UsamaHameed]

Changes

• Sets an implicit response time (via ctx.delay()) to 5ms, when run in a NodeJS environment.

GitHub

• Closes Using "ctx.delay" without arguments should add a realistic server response delay #192

[andreawyss]

I’m not sure that no delay at all is the correct approach.
Some asynchronous tests may fail.
We are mocking a server that in reality will have a delay.
We make the delay short so that unit tests run fast. But removing the delay may cause some unit test to fail.

For example calling an API and asserting that the spinner is showing may not work if the result is returned right away with no delay.

I think that in node the default delay should be some small amount.
5 or 10 ms.

Additionally it would be nice if one could override the default delay amount
with an option in server.listen() and worker.start().

[kettanaito]

Hey, @andreawyss. Those are definitely valid concerns. I'll elaborate below.

We are mocking a server that in reality will have a delay.

I think conceptually, we are making a server as a given valid functionality for our tests, so our testing focus can shift to the units we are testing (React components, helper functions, etc.).

For example calling an API and asserting that the spinner is showing may not work if the result is returned right away with no delay.

To test a loading state I wouldn't use implicit default delay at all. As its value is random, you are subjecting your test to race conditions and coupling it with MSW's internal default response delay time. I would rather advice to set an explicit long delay for testing a loading state.

Our main concern is that even 5ms of default response time in Node can become minutes when a big amount of tests is executed. Perhaps it's exaggerated, but I find the usefulness of realistic server response time in E2E tests and debugging, so the mocking feels natural, while in unit/integration tests I think the focus is to assert the right state.

What do you think about that?

Additionally it would be nice if one could override the default delay amount

We should definitely consider this!

[andreawyss]

If the default delay amount can be overridden then I will always set it to at least 1ms when running in node.
I believe that it is wrong to have the default to be 0ms when running in node.
We need to ensure that mock call remains async and that promises are not resolved right away.

[kettanaito]

@UsamaHameed, @andreawyss, what do you say if we make the default response time equal 5ms?

[andreawyss]

5ms is a good default value for when mocks run in node.

Detecting if the environment is node by testing (typeof window === 'undefined') may not work when using jest-dom.
Please use a better way to determine if the code is running in node vs. inside the service worker.

[kettanaito]

That's a good question: when you run a test in jsdom environment, do you expect a tested component to behave as in actual DOM (implying realistic server response), or being isolated, as in not focusing on the realistic network communication?

[andreawyss]

It doesn't matter. From a default behavior there are only 2 scenarios:

1. Run the App in the browser with service worker to simulate a real API with a delay that allows the loading transitions to be seen; Creating a UX that is as close as possible to using real APIs.

2. Run tests (unit and integration) in node as fast as possible while maintaining the mocked APIs asynchronous with a minimal delay. Tests can expect an intermediate state to exist between a pre-condition state and post-condition state.

When running in node we assume we are in a test environment and not in a Server-Side Rendering App.

[UsamaHameed]

I have set the response delay in node to 5ms.

[kettanaito]

@UsamaHameed, thanks for the great work!

I believe Andy is right saying that using typeof window is not reliable to detect if ctx.delay() executed in Node. In jsdom (browser-like) env, window will be polyfilled, and will exist, while we still should use Node response time.

Maybe we should base the conditional logic on typeof process instead? process indicates a NodeJS process, is not defined in a browser, and is defined in any Node process (regardless if it's Jest with jsdom/node, or a plain Node process). What do you think?

This change to the conditional logic also implies that we'd have to move the testing of the random server response time to the integration test (to run in an actual browser), but leave those two *.test and *.node.test test suites, but making them both assert the Node response time.

[citypaul]

Just a thought here - I noticed the conversation around using a different mechanism for determining whether the process was running inside node.

There's a library here that does this: https://github.com/iliakan/detect-node

Their mechanism is very simple though and it's just this:

  
// Only Node.JS has a process variable that is of [[Class]] process
module.exports = Object.prototype.toString.call(typeof process !== 'undefined' ? process : 0) === '[object process]';

Perhaps you could either just pull this lib in, or just directly inline this function?

If you do inline this function, it might be worth extracting it as a function named detectNode or something equivalent, just because this might turn out to be useful for other functionality in the project.

Just a thought!

[kettanaito]

Hey, @citypaul. Thanks for the suggestion!
I think we can start by including that snippet directly as a utility function in the library. You're right, we may want to parametrize the behavior in other parts, depending on whether it's running in Node or not.

[kettanaito]

Hey, @UsamaHameed. Please let me know if you need any help with these changes!

[UsamaHameed]

Hey, @UsamaHameed. Please let me know if you need any help with these changes!

I am a little swamped with some work. I will get back to this very soon. Thank you!

[kettanaito]

No rush, approach this only when you have time. Thank you for your contributions!

[kettanaito]

@UsamaHameed, may I please ask you to wait a little with these changes?

It appears that #219 will introduce the isNode utility function that checks if we are running in a Node environment. Could you please wait until that pull request is merged, and reuse that newly introduced function? Thanks.