storacha / freeway

🛣 Experimental IPFS HTTP gateway providing access to UnixFS data via CAR CIDs.
Other
14 stars 5 forks source link

feat: rate limiter + unit tests + readme #115

Closed fforbeck closed 1 month ago

fforbeck commented 1 month ago

This PR add tests for the Rate Limiter middleware, the main changes include:

travis commented 1 month ago

Could you say more about why Jest is useful here? We've typically used node:test for this project, and Mocha (https://mochajs.org/) or AVA (https://github.com/avajs/ava) on other projects - I'm not entirely sure why we've shied away from Jest, but I am a bit hesitant to add yet another testing framework to the mix for our team. Could definitely be convinced - I've used Jest in the past and it's generally pretty good - but need a bit more convincing ;-p

fforbeck commented 1 month ago

Could you say more about why Jest is useful here? We've typically used node:test for this project, and Mocha (https://mochajs.org/) or AVA (https://github.com/avajs/ava) on other projects - I'm not entirely sure why we've shied away from Jest, but I am a bit hesitant to add yet another testing framework to the mix for our team. Could definitely be convinced - I've used Jest in the past and it's generally pretty good - but need a bit more convincing ;-p

sure thing @travis

I don’t have super strong opinions on this either, but Jest does give you some nice built-in features like mocking, spies, and coverage reports right out of the box. With Mocha (or node:test), you’d need to add additional libraries like Sinon for mocks and nyc for coverage, which Jest handles natively.

That said, if we can use Mocha alongside node:test, it should work fine, even though Mocha isn't as feature-rich in those areas. Totally open to whichever approach the team prefers! 😊

travis commented 1 month ago

ok cool - I lean toward Mocha, but tbh I'd like @alanshaw to weigh in since he's written a lot more of this code (like, infinitely more, as this is really my first freeway contribution as well!) - Alan any preferences/thoughts on testing frameworks here?

alanshaw commented 1 month ago

Could you say more about why Jest is useful here? We've typically used node:test for this project, and Mocha (https://mochajs.org/) or AVA (https://github.com/avajs/ava) on other projects - I'm not entirely sure why we've shied away from Jest, but I am a bit hesitant to add yet another testing framework to the mix for our team. Could definitely be convinced - I've used Jest in the past and it's generally pretty good - but need a bit more convincing ;-p

sure thing @travis

I don’t have super strong opinions on this either, but Jest does give you some nice built-in features like mocking, spies, and coverage reports right out of the box. With Mocha (or node:test), you’d need to add additional libraries like Sinon for mocks and nyc for coverage, which Jest handles natively.

That said, if we can use Mocha alongside node:test, it should work fine, even though Mocha isn't as feature-rich in those areas. Totally open to whichever approach the team prefers! 😊

Slightly my bad here, I tried node --test because it was new, but turns out it isn't great so I'd welcome a switch to something else. We also don't use it in any other repo.

I'm mildly against having 2 testing frameworks in the same repo, but at the same time I don't want to waste time here - there's a bunch of tests written now using it so...meh. The point is more that testing exists at all - I don't mind which framework we use. People like consistency though (thinking about the team mostly here as I have a high tolerance for chaos), and using the same framework(s) everywhere allows us to work more quickly and without context switches. We should establish a single/set of frameworks we're comfortable using as a team. I realise that may have changed now. @travis this is something needed to be decided for team norms.

Just a couple of notes: 1) I though jest was legacy/unmaintained? 2) We don't use jest anywhere else.

Peeja commented 1 month ago

FWIW, I'm highly in favor of Jest. It's not legacy; quite the opposite, AFAIK it's where the community primarily is right now. Granted I'm biased by being used to Jest, but I'd love to decide to (in the long run) move everything to Jest.

fforbeck commented 1 month ago

@alanshaw I've applied pretty much all the suggestions, however, I left some of them to be done in different PRs to minimize the number of changes here. TODO