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

feat: v7 #580

Merged
merged 36 commits into from
Apr 30, 2024
Merged

feat: v7 #580

merged 36 commits into from
Apr 30, 2024

Conversation

wolfy1339
Copy link
Member

BREAKING CHANGE: package is now ESM
BREAKING CHANGE: remove type "oauth" that was previously deprecated

BREAKING CHANGE: package is now ESM
BREAKING CHANGE: remove type "oauth" that was previously deprecated
@wolfy1339 wolfy1339 added Type: Feature New feature or request Type: Breaking change Used to note any change that requires a major version bump labels Feb 24, 2024
Copy link
Contributor

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@wolfy1339
Copy link
Member Author

There's 2 tests that are failing, I don't know why

@wolfy1339
Copy link
Member Author

In the test: "auth.hook(): handle 401 in first 5 seconds (#65)",

It seems that the request to GET /repos/octocat/hello-world is not resolving. Something is preventing it from resolving in 5 seconds...

@gr2m Do you have any idea why the test would fail?

@harryzcy
Copy link

It seems the timeout error is intermittent. It's passing on some of the runs

@wolfy1339
Copy link
Member Author

It seems the timeout error is intermittent. It's passing on some of the runs

There is only one commit that has "passed checks" because the tests did not run on that commit

@gr2m
Copy link
Contributor

gr2m commented Apr 30, 2024

I spent some time trying to figure it out. I remove the custom sinon fake timers and use Jest's own instead. But things are still not working reliably. I think this is more of a jest problem than a problem with our code or tests. I really don't like using Jest for testing because it does to much "magic". I feel it's made for front-end testing and it's great for that, but for backend, I'd either use ava or Node's built in test runner.

@gr2m
Copy link
Contributor

gr2m commented Apr 30, 2024

I still see the tests that use the jest.advanceTimersByTimeAsync() API fail like once out of 7 runs. Honestly at this point I'd tear out jest, and for the time being live with re-running CI when the tests fail

@gr2m
Copy link
Contributor

gr2m commented Apr 30, 2024

I guess the CI doesn't like it 🤷🏼 shall we just skip the two tests that cause trouble to unblock v7 and get back to it later?

@wolfy1339
Copy link
Member Author

Fine with me

@gr2m gr2m marked this pull request as ready for review April 30, 2024 17:03
@wolfy1339 wolfy1339 merged commit 3e4d477 into main Apr 30, 2024
7 checks passed
@wolfy1339 wolfy1339 deleted the beta branch April 30, 2024 17:05
Copy link
Contributor

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants