incompatible behaviour introduced with v13.3.2

[andreas-roessler]

Please avoid duplicates

• I checked all open bugs and none of them matched my problem.

Reproducible test case

Code snipped added to "What happened?"

Nock Version

v13.3.2

Node Version

18.15.0

TypeScript Version

No response

What happened?

Hi dear nock-team,

with commit
8aab603
a new breaking change/behaviour has been introduced which crashes our tests of several projects.
It is reordering the interceptors which leads to a different sequence handling.

'use strict';

// Console with nock 13.3.2 (incorrect)
//   special file
//   all others
//   all others
//
// Console with nock 13.3.1 (correct)
//   special file
//   special file
//   all others


const https = require('https');
const nock = require('nock');
nock.back.setMode('lockdown');

const get = uri => {
	return new Promise(resolve => {
		https.get(uri, res => {
			const data = [];
			res.on('data', chunk => data.push(chunk));
			res.on('end', () => resolve(Buffer.concat(data).toString()));
		});
	});
};

(async() => {
	nock("https://test.com")
		.persist()
		.get("/special-file.json").reply(200, "special file")
		.get(/.*/).reply(200, "all others");

	console.log(await get("https://test.com/special-file.json"));
	console.log(await get("https://test.com/special-file.json"));
	console.log(await get("https://test.com/other-file.json"));
})();

Would you be interested in contributing a fix?

• yes

[gr2m]

can we close in favor of #2501? Maybe you and @andreas-roessler could work on a PR?

[andreas-roessler]

hi, as far as I can see #2501 has nothing todo with the problem I reported. The solution of the compatibility issue I reported is to revert 8aab603 which changed the order of interceptors after execution and with that the precedence. The new behaviour is not a side effect of the commit merged by @mikicho. It is the intended behaviour which is unfortunately incompatible and lead to plenty of problems in our testing zoo.

[mikicho]

My 2 cents.

I tried to solve two problems:

1. Inconsistency behavior when using persist. The persisted nock is not always at the top of the stack until it gets there.
2. persist violates Nock's stack principle because persisted nocks are stuck at the top of the stack.

@andreas-roessler It appears that the example in the ticket is a simplified version of your current setup. Can you generate nocks only when needed instead of always saving the full scope?

[andreas-roessler]

hey @mikicho,
Yes ... right.
We are working on a product with a bunch of tests which are more complicated than the provided example.
I just debugged the failing test pipeline of our blocked release and found out what the actual root cause was.
Therefore I would say that 
I’m not really familiar with the internal mechanisms of nock.

We often use nock in a “full-persistent-mode” by setting "persist()" as first statement. This is to simulate a whole server file structure for several unit-tests in row.
I would assume that if running all interceptors in persistent mode no nock-principle is violated by the old behaviour. We set quite often “persist()” as first statement as far as I can see in the tests I scanned.
For us this was a breaking change.

[KrayzeeKev]

I agree with @andreas-roessler - we use nock to create an "emulator" of a remote system. Then we hit it with multiple requests. We need one response for /foo/bar and another for the rest of /foo/* - if the latter triggers for /foo/bar, it's all broken. So we define it in the order it needs to match. Always have and it's worked well.
Redefining the entire thing for every test case means globalising a whole lot of variables and wasting a large amount of processing, etc.
Effectively, we're saying that persist is no longer usable. You have to redefine all nocks every time. And even that isn't practical, as a single test may have 2 or 3 related operations in it so we do: nock(), POST, nock(), GET, nock() GET /:id - it's a mess.

[gr2m]

Shall we do this then?

1. Create PR to revert the change of v13.3.2
2. Create PR that adds a test that will prevent the same regression from happening again

[mikicho]

IMO this is an anti-pattern, and they should use tools like Stubby, which does EXACTLY that.

But if we decide to revert the change, I recommend adding an option to the persist function which enables new behavior and consider deprecating the anti-pattern behavior on the next major.

[KrayzeeKev]

Stubby looks interesting but it appears to require spinning up an external process does it not? Which is painful. And it can't do the one thing we require of nock and that's to be dynamic. I need to POST to create something, get back a random uuid and then be able to GET that uuid. To test CRUD operations on our end. Otherwise you end up having to start from scratch for each subpart of each test.

I'm interested in finding out what is driving this change and what the anti-pattern is? If you have multiple endpoints in a hierarchy you need to know the deeper ones will match in preference to the higher level ones. Why is it bad to have the interceptors run in the order they're specified? Bad enough to warrant changing it and even worse than that proposing to deliberately remove that in a major release.

I would normally say "if it's not broken..." so I'm keen to understanding why it's considered broken because, in its current form, the entire module appears unusable to us. @mikicho ? Genuine question - wanting to understand. (Mainly because we might be doing something wrong or poorly and could mitigate on our end by doing it right - other than recreating all the nocks between multiple steps of a single test case)

[KrayzeeKev]

I think I might have found out why this change was messing with us so much. Seems my guys were abusing regular expressions. Some adding of ^ and $ and [^/]* instead of .* and the occasional /? on the end (grrrr) to construct some far more exact matches seems to have solved this for us. Or at least we're doing things better so ordering changes won't affect us. Which is what we always should have been doing.

Still interested in the pattern / anti-pattern discussion, though - unless you meant exactly what we were doing.

[andreas-roessler]

I’m quite sure that I’m not aware of the full potential of nock and its concept but it seems to me that a mixture of persistent and non-persistent interceptors is hard to handle in principle. With the new behavior I wonder in which mixture scenario a persistent interceptor is a good choice without getting completely confused.
May two logical stacks with precedence on non-persistent-interceptor-stack might be a solution but it would introduce yet another concept. For us it would work out of the box.

If there is no more resistance from consumer's-side within the next weeks, it is ok for me to invest the effort of some days to adapt our unit tests by defining more specific path-matchers with extremely long Regexps.
Anyway, I have the opinion that such changes shouldn’t be made on patch but on major release level.
I would appreciate a “compatibility” option to switch back to the old behavior. With that the consumers have at least a kind of grace period without loosing the possibility to consume (security) fixes.

[KrayzeeKev]

We found that ordering was changing when everything was persistent, I'm sure.

[mikicho]

Stubby looks interesting but it appears to require spinning up an external process does it not?

It was just an example. You can use json-server or httpmock, which is very similar to nock and maybe what you are looking for.

Potential better suite alternative list: https://www.npmjs.com/search?q=keywords%3Aserver%2C%20stub

I'm interested in finding out what is driving this change and what the anti-pattern is?...

Unlike other tools, Nock uses a stack to manage its interceptors, as noted here, interceptors are removed once they have been used.
IMHO every test should create and consume its own interceptors and call to expect(nock.isDone()).toBeTruthy() at the end to make sure all request get consumed as expected, otherwise, there is a chance the test succeed even the interceptor not even called. Moreover, it decreases the mental effort a developer needs to read/update/fix the tests because they need to deal with a handful of concrete interceptors. This is one of the main reasons I migrated from tools like Stubby or json-server.
The usage of persist should be little and only when to response is consistent between tests, e.g. authorization.

think I might have found out why this change was messing with us so much.

😅 I wrote the previous phrase before reading this comment... I was there... this is why I stopped using the "whole server mock" approach, the debugging of a whole server with complex regex was painful.

Anyway, I have the opinion that such changes shouldn’t be made on patch but on major release level.

I agree, and I'm sorry about it. But sincerely, I recommend you either embrace nock philosophy (big yes!!) or use other tools which suit better your needs and may have other features the nock doesn't.

[gr2m]

Anyway, I have the opinion that such changes shouldn’t be made on patch but on major release level

Just to be clear: it was not a matter of opinion, but a matter of insufficient test coverage, hence the ask for a test to avoid the same regression in the future.

[andreas-roessler]

I agree, and I'm sorry about it. But sincerely, I recommend you either embrace nock philosophy (big yes!!) or use other tools which suit better your needs and may have other features the nock doesn't.

We have a mixture of persistent and non-persistent setups (even within one test file) depending on the unit test.
IMO a mix of tools (nock, stubby etc) is a kind of awkward. They might interfering. The knowledge of several test tools must be distributed to the whole dev team. We chose nock several years ago because it can handle both approaches.

I still do not know a practical use case in which a mix of persistent and non-persistent interceptors make sense, without getting confused with the new approach.

Just to be clear: it was not a matter of opinion, but a matter of insufficient test coverage, hence the ask for a test to avoid the same regression in the future.

Does this mean that the there is no doubt about it that this change is a breaking change an should be reverted for this major version and an additional regression test has to be added to avoid such things happen again in future?
If this is the case we have a conflict, which I do not know to handle.
According to @mikicho I understood that we (our dev team) should adapt our unit tests to the new behaviour because the concept of nock is complete different to that what we assumed some years ago.
According to @gr2m the regression has to be fixed (if I understood the comment correctly).

What is the next action?

I'm currently not allowed to contribute yet until I have finished a company-internal OS contribution training (definitely will do that eventually)

[geofholbrook]

2 cents from me as well:

I'm open to understanding the reason for the change, and also try to embrace nock's philosophy :)

Still, for now: we use "full persist mode" and have a catch-all interceptor, so after upgrading from 13.3.0 to 13.3.2, the second call to a specific endpoint is intercepted by the catch-all. That's why this patch version breaks a lot of tests for us. I would echo that the next action should be to revert the change for this major version.

[gr2m]

I still think these are the next steps.

• Create PR to revert the change of v13.3.2
• Create PR that adds a test that will prevent the same regression from happening again

I'll do a revert PR for v13.3.2 now as we seem to agree to do that.

@mikicho if you depend on the nock.removeInterceptor fix you can pin the version to 13.3.2 until we figure out how to implement it without the regression.

[mikicho]

We reverted this change. We can close this one.
Feel free to reopen if needed.