-
Notifications
You must be signed in to change notification settings - Fork 65
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
[MAINT]: Revisit skipped tests #441
Comments
👋 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 labeled with |
Copying over discussion on the tests:
TL;DR (in
|
Request | Response |
---|---|
/repos/OWNER/REPO/branches |
200 |
/repos/OWNER/REPO/branches/ |
404 |
But, /repos/OWNER/REPO/contents/PATH
works with and without /
, so we should probably support both 1.
Request | Response |
---|---|
/repos/octokit/endpoint.js/contents |
200 |
/repos/octokit/endpoint.js/contents/ |
200 |
/repos/octokit/endpoint.js/contents/test |
200 |
/repos/octokit/endpoint.js/contents/test/ |
200 |
This mismatch comes when we compare the requested URL (with no ending /
) and the expected from @octokit/fixtures
(with ending /
):
https://github.com/octokit/fixtures/blob/5aa66e46e3f3475c4c1eb521cd402dfd85091b52/scenarios/api.github.com/get-content/normalized-fixture.json#L5
We should probably improve the logic we have in place in @octokit/endpoints
, but I would like to hear your thoughts on this @octokit/js @octokit/extensibility-sdks. I'm discarding the option of fixing it in @octokit/fixtures
because those are auto-generated.
The Problem (in test/scenarios/get-archive.test.ts
)
The error prompted is the following one (complete logs here):
HttpError: Unknown error: {"error":"Nock: No match for request","detail":{"expected":{"method":"GET","url":"https://fixtures-r9di1toxolb.api.github.com/repos/octokit-fixture-org/get-archive/tarball/main","headers":{"accept-encoding":"gzip, deflate","sec-fetch-mode":"cors","accept-language":"*","authorization":"token 0000000000000000000000000000000000000001","user-agent":"octokit-rest.js/0.0.0-development octokit-core.js/6.1.2 Node.js/20.12.1 (darwin; x64)","accept":"application/vnd.github.v3+json","connection":"close","host":"fixtures-r9di1toxolb.api.github.com"}}}}
The root cause
I've been debugging a bit but did not found the root cause yet. I will keep checking. It's the only fixture which uses 302
as status response. Maybe it has something to do with that. What I can say is the issue is unrelated to the one with get-content
.
Footnotes
-
Apparently in the past the trailing
/
was not working for/repos/{org}/{repo}/contents
endpoint but seems that it was escalated and now it's working. ↩
Just came here to say thank you, this is really good work @wolfy1339! |
Describe the need
Some tests were failing for unknown reasons due to the switch to ESM and were skipped
SDK Version
No response
API Version
No response
Relevant log output
No response
Code of Conduct
The text was updated successfully, but these errors were encountered: