-
Notifications
You must be signed in to change notification settings - Fork 30
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
Test Suite written in Jest #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful @KonoMaxi 👏
I left some comments with some small details and I think that yes, we can and should include it in the CI, can you do it in this PR? In theory, you just need to add 2 lines to the end of this file https://github.com/rails/request.js/blob/main/.github/workflows/ci.yml like is done to the lint step, something like
- name: Test
run: yarn test
request = new FetchRequest("get", "localhost") | ||
expect(request.fetchOptions.redirect).toBe("follow") | ||
|
||
// maybe check for valid values and default to follow? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the default being checked in the prior check? About checking valid values, I don't think that's necessary, since Fetch API will raise an error if the user provides an invalid value. At least I think that we would be testing Fetch API behavior and not Request.JS behavior. Having a test that proves that it's possible to override the default value provided by request.js is enough, like the test bellow (even if that isn't a valid value)
After writing my first Jest Test-Suite, I didn't feel like I understood the framework, so I looked for another Project to try it on. That's when I noticed Request.js has no coverage yet.
Learned a bit on the journey, so if you wanna include it in your CI, you're free to do so :-)