Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update test framework #767

Draft
wants to merge 5 commits into
base: devel
Choose a base branch
from

Conversation

nocodehummel
Copy link
Contributor

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
Copy link
Contributor Author

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

@nocodehummel nocodehummel marked this pull request as draft April 18, 2024 18:00
@gadicc
Copy link
Owner

gadicc commented Apr 19, 2024

😮

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

@gadicc
Copy link
Owner

gadicc commented Apr 19, 2024

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

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! 🙏 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 😅

Thanks again!

@nocodehummel
Copy link
Contributor Author

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
Copy link
Contributor Author

nocodehummel commented Apr 20, 2024

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
Copy link
Owner

gadicc commented Apr 24, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

gadicc commented Apr 24, 2024

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 🙏

@gadicc
Copy link
Owner

gadicc commented Jun 17, 2024

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
Copy link
Contributor Author

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

@gadicc
Copy link
Owner

gadicc commented Jun 19, 2024

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 🙏 🌞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants