Mocked request starts timing out after picking up v10.0.5

[Chengxuan]

What is the expected behavior?
A minor version update of nock library shouldn't impact my existing tests.
What is the actual behavior?
All my tests uses fakeTimers are now timing out.
Possible solution
Revert the fix for #677, or make it optional if it has to go as a minor version update.
How to reproduce the issue
Have sinon.useFakeTimers() in the test case.

Having problem producing a test case? Try and ask the community for help. If the test case cannot be reproduced, the Nock community might not be able to help you.

Does the bug have a test case?

Versions

Software	Version(s)
Nock	10.0.5
Node	8

@gr2m @taylor1791

[paulmelnikow]

This could have been communicated better, our tests started failing after picked up 10.0.5 because most of them use fakeTimer which didn't impact nock. Now they all timing out and it took me a while to figure out why. I agree to give more control is better, but I don't think most of existing users know it uses a separate timer due to existing behaviour. They'll need to go through the same debugging process as I did now.

Maybe update your readme with an example would help? This is a change of behaviour for all the existing tests uses fakeTimer. Those tests will start failing with no obvious error other than timing out and it would take a while to track down to the nock library.

Hi! Sorry about the difficulty and thanks for opening an issue. Do you mean an example of using fake timer? Sinon's fake timers? I'm not familiar with using fake timers with nock. Could you create an example?

Adding a test would be really helpful. It would document the behavior and prevent a future regression. Your help with that would be really welcome :)

[humantree]

I'm also encountering this issue. Here is a simple example of it occurring: https://github.com/humantree/nock-fake-timers

This test passes on 10.0.4, but fails on 10.0.5.

[Chengxuan]

@gr2m @taylor1791 would appreciate your opinions on this issue since the change was introduced by you. I wonder should the behaviour of this library be affected by using fake timer. Because the real http request does not seem to be affected by timer (It's managed in a different phase of the node js event loop.).
This library happens to use timer to simulate the http response but it doesn't mean this implementation details should be exposed and exploited by the consumer of this library. Plus, this change does increase the complexity of using this library and break existing behaviour. I'd suggest it to be rolled back.

I have to put comments in my test code to tell the reader why I'm manipulating the fake timer in some special lines. This is really a pain... Readability and easy to use should be considered.

https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/

[taylor1791]

Regarding the statement:

This library happens to use timer to simulate the http response but it doesn't mean this implementation details should be exposed and exploited by the consumer of this library.

I completely agree, but I think that I think that should be discussed in a different MR. In addition, it is certainly a breaking change if !677 is.

If the maintainers of nock deem it appropriate, I am happy to help contribute a fix. I think there are a few choices to fix it, but I agree with @chenghung. I think the best "fix" is to revert, mark it as a breaking change, and re-publish this change in v11. However, I can think of a few reasons the maintainers might not desire this.

I will defer to the maintainers if they think we should fix this. I can see it 2 ways.

1. If you arbitrarily monkey patch the execution environment, no library can provide backwards compatibility guarantees.
2. Monkey patching clocks in the execution environment is common enough that nock should guarantee backwards compatibility when mocking clocks.

First, assume that all assertion frameworks using fake timers equally. If we knew what percent of javascript projects that use sinon, also use fake timers, we could support or reject 2. I would say that if >4% of projects use fake timers, then view 2 is supported. Others are welcome to submit different percentages. I tried to collect some data using open source projects, but it seemed inconclusive to me. If you are interested in the inconclusive results keep reading.

Using 'mocha' filename:package.json extension:json language:JSON as the search criteria across github, I found 1,200,331 million files. Since most projects are not likely to have more than one package.json, we can assume there are about 1.2 million projects using mocha.

Searching for sinon useFakeTimers extension:js language:JavaScript on github provides 90,904 files. Searching for lolex install extension:js language:JavaScript on github provides 11,166 files. This might be a close proxy for number of files (not projects) using fake timers. Since this is an upper bound for the number of projects using fake timers, we can pretty safely say that this will break no more than 8.5% of projects using mocha.

If we assume that jest users use mock timers about the same as mocha users, we can another data point. Using jest filename:package.json extension:json language:JSON as a search result we get 410,030 projects using jest. Using jest useFakeTimers extension:js language:javascript as a search result we get 5993 files using fake timers. So jest, has a upper bound of 1.5% projects breaking using this change.

It is possible that projects using any testing framework, sinon, and fake timers do not break (for various reasons). However, this only increases makes the reported percentages higher upper bounds. If we want to go further, someone could try to clone a fraction of repositories and try to find failures after upgrade nock, but that is fraught with perils.

Looking at the quality of the search result, it is unclear which one is "closer" to reality. Perhaps we could use activity on this issue as a gauge for the number of projects updating that broke, but I am not sure how to gage total projects that upgraded without problems. To go further, one try to clone a group of repos

[Chengxuan]

@taylor1791 you made good points there. We'll see how maintainers respond. Also worth pointing out, not all repos are on the public github, there are also ones on other souce code management systems.

[paulmelnikow]

I can see the argument that this should be a breaking change. It begs the question though: is the new behavior desirable?

@humantree thanks for the example:

const axios = require('axios');
const { expect } = require('chai');
const nock = require('nock');
const sinon = require('sinon');

describe('Test', () => {
  const url = 'http://www.google.com';
  let clock;

  before(() => clock = sinon.useFakeTimers());
  after(() => clock.restore());

  beforeEach(() => nock(url).get('/').reply(201));
  afterEach(() => expect(nock.isDone()).to.equal(true));

  it('should pass this test', async () => {
    const res = await axios(url);
    expect(res.status).to.equal(201);
  });
});

How would you take advantage of fake timers in a test with nock?

@taylor1791 do you have an example where fake-timing nock's internal timer was desirable?

[humantree]

In my case, the test that broke was testing a library that hit a list of URLs multiple times a second. In order to prevent this, I was using Sinon to freeze the timer so that I could use Nock to fake these URLs and be sure they were only hit once.

[Chengxuan]

@paulmelnikow I'd like to address that if you only use fake timer without using nock in a test and let the application call out to the real http endpoint, you DO NOT need to manipulate the timer to make the test receiving those http responses (example test: https://github.com/Chengxuan/http-response-test/blob/master/test.js).

I'd encourage whatever new capability introduced by this change should be exposed as a nock option instead of exposing the internal implementation. it should at least be controlled by nock directly, e.g. nock.useFakeTimers() instead of sinon.useFakeTimers().

However, the handling of https responses is not affected by timers at all in node.js . This change can misguide the users of nock library to have a wrong understanding of node.js event loop. Therefore, I think it should be removed. I could not think of a good use case for this at the moment (https://github.com/nock/nock#delay-the-response already supports most imagined use case I've come up with).

[Chengxuan]

@paulmelnikow @taylor1791 @humantree To make sure we don't go too far away from the original issue. Can we work on fixing this first by reverting the change if we all agree on the following points:

1. a minor version update shouldn't change the existing behaviour of the library
2. the change of the behaviour is introduced as a side effect of fixing a code consistency issue
3. the change of the behaviour doesn't seem to be backed up by any real use case yet since @paulmelnikow has asked what the use case is

A new issue should be raised against this repo to figure out the use case of this side effectsince I'm not sure everyone looking at this issue are interested in that discussion.

[gr2m]

I'm not yet sure if this is a breaking change? The behaviour that broke for you was never documented or tested. But as this affects so many people I’d be okay with reverting it for now.

We can release it as a breaking change, but what would the migration path be? I use lolex whenever I need to mock time and never had problems with it when using the .install() method, which overrides the native timers.

[Chengxuan]

@gr2m lolex is extracted from sinon. The same issue can be recreated with lolex as well. Here is the same broken test from @humantree using lolex instead of sinon fake timer: https://github.com/Chengxuan/nock-fake-timers/blob/master/test/test.js and the issue is still there. The test passes with 10.0.4, but not 10.0.5. I agree that a test should be written to avoid regression in the future, there's a test gap here.

[gr2m]

Thanks @Chengxuan.

Could someone send a PR to revert #677? We will release it as a fix release

[paulmelnikow]

It'd be helpful to include a test, as well, that lolex.install() does not interfere with nock.

[Chengxuan]

I'll work on a PR then.

[nockbot]

🎉 This issue has been resolved in version 10.0.6 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[nockbot]

🎉 This issue has been resolved in version 11.0.0-beta.3 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

[nockbot]

🎉 This issue has been resolved in version 11.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀