Update test framework

[nocodehummel]

Refactor test framework #766 to improve TDD and untangle test code from actual code.

Changes

• dependency on jest-fetch-mock and jest-fetch-mock-cache
• minor refactoring of cookie related code

Type

• [ ] New Module
• [X ] Other New Feature
• [ ] Validation Fix
• [ ] Other Bugfix
• [ ] Docs
• [ ] Chore/other

Comments/notes

In the getCookies sample the source code has no test related logic or test related properties. The test case can be run independently via command line or JEST Runner. API testing can focus on the test cases and does not need to worry about the caching (naming etc).

[nocodehummel]

@gadicc hereby a sample for the changes to the test framework. The JFMC package is really useful for API development!!

[gadicc]

:open_mouth:

I'm a little in shock at how quickly you did this!! Reviewing now :pray: :pray:

[gadicc]

Thanks @nocodehummel, looks like we're off to a great start here :pray:

I must admit, I had never actually thought about having fetchDevel and JFMC code co-exist. It's actually a great idea that will let us progressively switch over the project piece by piece, which is a lot more manageable. So thanks :)

All looks good so far. But not sure if the cookie code will work with multiple Set-Cookie headers. I did see your note about about the new node version, but that still requires the special Headers#getSetCookie, and also, we need to support all currently supported node versions, of which the minimum is currently still v18. But more importantly, we're actually using the node-fetch package under the hood, and that needs headers.raw() for this case.

The JFMC package is really useful for API development!!

Thanks so much! :pray: Yeah, fetchDevel worked so well for yahoo-finance2 that I wanted a similar flow for other projects, but more generalized and as a jest mock. I use it in a bunch of packages and it's such a big step up from older workflows. I should probably try to promote it more :sweat_smile:

Thanks again!

[nocodehummel]

But more importantly, we're actually using the node-fetch package under the hood, and that needs headers.raw() for this case.

Node.js 18 is now active maintenance and Node.js 16 is no longer supported. I think this opens for using the Node global (experimental) fetch API instead of node-fetch. All our test cases are passing using the fetch API (Node v18.18.2). I will commit a change for you to check.

I had a quick look at the usage of different environments and it looks like that is related to the fetch API. Is there any other reason?

[nocodehummel]

The usage of fetchDevel is a very good approach with dependency injection to separate concerns and improve function testing. Preferable it should not impact the implementation of a function as in below code or its interface. I would like to achieve with this PR that the injected dependency (for testing) does not impact implementation.

// no need to force coverage on real network request.
const fetchFunc = moduleOpts.devel ? await fetchDevel() : fetch;

const fetchOptionsBase = {
  ...moduleOpts.fetchOptions,
  devel: moduleOpts.devel,
  headers: {
    "User-Agent": userAgent,
    ...moduleOpts.fetchOptions?.headers,
  },
};

[gadicc]

Yeah, I'd love to move off node-fetch. And indeed, we only need to support Node 18+ now. But Headers#getSetCookie() is only available in Node 19.7.0 as per the compatibility matrix at the bottom of that page.

Re different environments, it was to cover browser vs node. fetch was definitely a big part of that, but there were some other native APIs too. I don't think we'll be able to continue supporting browser moving forward because of cookies, but, I'd love to keep supporting something close to it, for edge computing.

Sorry, I didn't quite follow your last comment re fetchDevel. But I think the plan is to replace it with jest-fetch-mock-cache right? But yes there may be some missing features we'll need to find a good way to implement there.

[nocodehummel]

Hi @gadicc, thanks for the feedback and looks like we are aligned. Yes, fetchDevel will be replaced by the jest-fetch-mock-cache. I had a look at getSetCookie but it is not in the code anymore. I plan to do some more work on this next week.

[gadicc]

Perfect! Thanks so much. Sure, no pressure whatsoever on my side, take your time, and thanks again :)

Yeah in yahoo-finance2 we don't use getSetCookie since we're using node-fetch which has it's own API, headers.raw()['set-cookie'].

In JFMC we inspect the headers object at runtime and so the user can use both node-fetch or native fetch (in later node versions). But in yahoo-finance2 since we only used node-fetch, we only used it's own API. When we switch to node's native fetch in the future, we'll have to then use getSetCookie() instead.

Thanks again, happy coding, and take your time :pray:

[gadicc]

Hey @nocodehummel, just checking in.

Absolutely no pressure from my side, I'm also swamped with stuff, just wanted to see how things are going your side and if I can help at all.

[nocodehummel]

Hi, there is has been too many other priorities, and holiday season started. I expect it will take me until after the summer.

[gadicc]

Thanks, @nocodehummel, that's perfect. Never any pressure from my side, just wanted to check in. Good luck with your other priorities, enjoy the holidays, and we'll touch base after the summer :pray: :sun_with_face: