Codecov Report

Merging #2952 (cf5dff5) into develop (dde8860) will decrease coverage by 42.07%.
The diff coverage is 70.43%.

@@             Coverage Diff              @@
##           develop    #2952       +/-   ##
============================================
- Coverage    65.21%   23.13%   -42.08%     
============================================
  Files           14       48       +34     
  Lines          733    10026     +9293     
============================================
+ Hits           478     2320     +1842     
- Misses         255     7706     +7451     

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Hmm, it's so strange that it only fails on 18.x, seems to be when it uses the built in fetch instead of node-fetch. Changing that and it works...

Hmm, it's so strange that it only fails on 18.x, seems to be when it uses the built in fetch instead of node-fetch. Changing that and it works...

running the tests locally with npm run test:e2e the one ipWhitelist test fails as in the automated tests here.

But running the single test with npx jest tests/e2e/ipWhitelist_spec.js works ...

Maybe something isnt shutdown correctly before the one test?

Maybe something isnt shutdown correctly before the one test?

yes, but it not depends on which test is running before, had different tests as predecessor ...

if I put back the ugly "wait" in stopApplication it works ...

exports.stopApplication = async () => {
        if (global.app) {
                return new Promise((resolve, reject) => {
                        global.app.stop(resolve);
                });
        }
        await new Promise((resolve) => setTimeout(resolve, 100));
        return Promise.resolve();
};

The fetch fails and there's no catch() in the wrapper in global-setup.js and that causes the test to wait forever for the resolve to happen (which it doesn't).

Using the following it fails more in a proper way:

exports.fetch = (url) => {
	return corefetch(url);
};

But haven't yet found out why the fetch fails with native in node 18 and why it works with node-fetch...

I wont be able to look into this for a week or so.

Changed the tests to use node-fetch instead of the built-in fetch for node >= 18 because the built-in sets the header Connection: keep-alive between all requests which messes with the server starting up and shutting down between all the tests. Tried setting the header manually and setting a separate agent without any success. And it always logs that fetch is still an experimental feature, so I think we should treat it as that...

Not sure if I requests @khassel to review, otherwise I would merge it

with this we are using internal node fetch command (for node >=18) in the default modules but the tests are running with node-fetch instead. Means for me these are no tests for the modules (for node >=18).

• If the tests call production code on node >= 18 the internal fetch will still be used, it's just the fetches that is used only in the test code that would use node-fetch
• I don't believe the internal fetch is production ready and should probably not be used at all (yet) because:
  • Running the following:

   node -e "global.fetch('http://en.wikipedia.org').then((res) => console.log(res.status))"

  Produces:

   (node:71384) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time
   (Use `node --trace-warnings ...` to show where the warning was created)
   200
  

  • There's no documentation for it on https://nodejs.org/dist/latest-v18.x/docs/api/
  • The issue it creates with lingering connections like in this test is probably not wanted in the production code either
• Having tests depend on a randomly selected timeout and sometimes fail is really annoying, had that happen to me while debugging this with the old 200ms timeout.

I don't believe the internal fetch is production ready and should probably not be used at all (yet)

that was my point, I don't like it when we migrate the tests back but the rest not. If the internal fetch is now seen as not production ready it should be removed completly (and may evaluated later again, it is still experimental in node v19)

but from my side we can merge this and remove internal fetch in another PR

I am not so deep into the whole node/fetch thing but maybe what is done in the other PR (1c8ea72)
would be a solution?

let nodefetch;
		if (nodeVersion < 18) {
			jest.mock("node-fetch");
			nodefetch = require("node-fetch");
		}
		let fetchMock;

But that is just my "gefährliches halbwissen" (dagnerously half-knowing :-)

Mocking the requests would probably make most of the tests useless as they verify the response from the server. Don't think that solution can be applied here.

I agree that it's better to do it in another PR.

Guess we have to wait a bit longer to get rid of that extra dependency :)

I agree. It would be nice to get rid of a dependency but we should play it safe.
So, whats the plan now? First a PR for getting rid of internal fetch, or first merge your PR?

you can merge.

The other change would be here. I would suggest to comment out the if/else (but leave it in) and return always the node-fetch part. So we have to change nothing else and can insert the internal or other fetch functions there when needed.

can do this PR the next days

can do this PR the next days

Or I do it on a saturday evening? #2961 ;-)