-
Notifications
You must be signed in to change notification settings - Fork 128
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
fix: replace searchAPI
usage with GraphQL
in findSRIssue
util
#907
Conversation
…er` to its params;
…-release/github into feat/gql-for-search-api
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.
BLOCKED HERE! HELP 🫨 📣
Everything works just as expected giving the PR a whirl.... But in the integration test coverage; the case below is bloody frustrating me 😭
github/test/integration.test.js
Lines 641 to 825 in 50c3f9c
test("Verify, release and notify success", async (t) => { | |
const owner = "test_user"; | |
const repo = "test_repo"; | |
const env = { GITHUB_TOKEN: "github_token" }; | |
const assets = [ | |
"upload.txt", | |
{ path: "upload_other.txt", name: "other_file.txt", label: "Other File" }, | |
]; | |
const failTitle = "The automated release is failing 🚨"; | |
const options = { | |
publish: [ | |
{ path: "@semantic-release/npm" }, | |
{ path: "@semantic-release/github", assets }, | |
], | |
repositoryUrl: `https://github.com/${owner}/${repo}.git`, | |
}; | |
const nextRelease = { | |
gitTag: "v1.0.0", | |
name: "v1.0.0", | |
notes: "Test release note body", | |
}; | |
const releaseUrl = `https://github.com/${owner}/${repo}/releases/${nextRelease.version}`; | |
const assetUrl = `https://github.com/${owner}/${repo}/releases/download/${nextRelease.version}/upload.txt`; | |
const otherAssetUrl = `https://github.com/${owner}/${repo}/releases/download/${nextRelease.version}/other_file.txt`; | |
const releaseId = 1; | |
const uploadOrigin = "https://github.com"; | |
const uploadUri = `/api/uploads/repos/${owner}/${repo}/releases/${releaseId}/assets`; | |
const uploadUrl = `${uploadOrigin}${uploadUri}{?name,label}`; | |
const prs = [{ number: 1, pull_request: {}, state: "closed" }]; | |
const commits = [{ hash: "123", message: "Commit 1 message" }]; | |
const fetch = fetchMock | |
.sandbox() | |
.get( | |
`https://api.github.local/repos/${owner}/${repo}`, | |
{ | |
permissions: { push: true }, | |
full_name: `${owner}/${repo}`, | |
clone_url: `htttps://api.github.local/${owner}/${repo}.git`, | |
}, | |
{ | |
repeat: 2, | |
}, | |
) | |
.postOnce("https://api.github.local/graphql", { | |
data: { | |
repository: { | |
commit123: { | |
oid: "123", | |
associatedPullRequests: { | |
pageInfo: { | |
endCursor: "NI", | |
hasNextPage: false, | |
}, | |
nodes: [prs[0]], | |
}, | |
}, | |
}, | |
}, | |
}) | |
.postOnce( | |
`https://api.github.local/repos/${owner}/${repo}/releases`, | |
{ | |
upload_url: uploadUrl, | |
html_url: releaseUrl, | |
id: releaseId, | |
}, | |
{ | |
body: { | |
tag_name: nextRelease.gitTag, | |
name: nextRelease.name, | |
body: nextRelease.notes, | |
draft: true, | |
prerelease: false, | |
}, | |
}, | |
) | |
.patchOnce( | |
`https://api.github.local/repos/${owner}/${repo}/releases/${releaseId}`, | |
{ html_url: releaseUrl }, | |
{ body: { draft: false } }, | |
) | |
.getOnce( | |
`https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, | |
[{ sha: commits[0].hash }], | |
) | |
.postOnce( | |
`https://api.github.local/repos/${owner}/${repo}/issues/1/labels`, | |
{}, | |
{ body: ["released"] }, | |
) | |
.postOnce( | |
(_, { body }) => { | |
t.regex(JSON.parse(body).query, /query getSRIssues\(/); | |
return true; | |
}, | |
{ | |
data: { | |
repository: { | |
issues: { nodes: [] }, | |
}, | |
}, | |
}, | |
) | |
.getOnce( | |
`https://api.github.local/search/issues?q=${encodeURIComponent( | |
"in:title", | |
)}+${encodeURIComponent(`repo:${owner}/${repo}`)}+${encodeURIComponent( | |
"type:issue", | |
)}+${encodeURIComponent("state:open")}+${encodeURIComponent(failTitle)}`, | |
{ items: [] }, | |
) | |
.postOnce( | |
`${uploadOrigin}${uploadUri}?name=${encodeURIComponent("upload.txt")}&`, | |
{ browser_download_url: assetUrl }, | |
) | |
.postOnce( | |
`${uploadOrigin}${uploadUri}?name=other_file.txt&label=${encodeURIComponent( | |
"Other File", | |
)}`, | |
{ | |
browser_download_url: otherAssetUrl, | |
}, | |
) | |
.postOnce( | |
`https://api.github.local/repos/${owner}/${repo}/issues/1/comments`, | |
{ | |
html_url: "https://github.com/successcomment-1", | |
}, | |
); | |
await t.notThrowsAsync( | |
t.context.m.verifyConditions( | |
{}, | |
{ cwd, env, options, logger: t.context.logger }, | |
{ | |
Octokit: TestOctokit.defaults((options) => ({ | |
...options, | |
request: { ...options.request, fetch }, | |
})), | |
}, | |
), | |
); | |
await t.context.m.publish( | |
{ assets }, | |
{ | |
cwd, | |
env, | |
options, | |
branch: { type: "release", main: true }, | |
nextRelease, | |
logger: t.context.logger, | |
}, | |
{ | |
Octokit: TestOctokit.defaults((options) => ({ | |
...options, | |
request: { ...options.request, fetch }, | |
})), | |
}, | |
); | |
await t.context.m.success( | |
{ assets, failTitle }, | |
{ | |
cwd, | |
env, | |
options, | |
nextRelease, | |
commits, | |
releases: [], | |
logger: t.context.logger, | |
}, | |
{ | |
Octokit: TestOctokit.defaults((options) => ({ | |
...options, | |
request: { ...options.request, fetch }, | |
})), | |
}, | |
); | |
t.deepEqual(t.context.log.args[0], ["Verify GitHub authentication"]); | |
t.true(t.context.log.calledWith("Published file %s", otherAssetUrl)); | |
t.true(t.context.log.calledWith("Published file %s", assetUrl)); | |
t.true(t.context.log.calledWith("Published GitHub release: %s", releaseUrl)); | |
t.true(fetch.done()); | |
}); |
Here's what I've got:
In the context of this particular test case, I have only made 2 changes, and they're stated below...
-
Introduced a graphql request mock to intercept the graphql request in
findSRIssues
; did this with a functionmatcher
option ensuring no conflict with the existing graphql request mock for fetchingassociatedPRs
.postOnce( // Matcher Starts (_, { body }) => { t.regex(JSON.parse(body).query, /query getSRIssues\(/); return true; }, // Matcher Ends { data: { repository: { issues: { nodes: [] }, }, }, }, )
-
THE SECOND CHANGE DYNAMICS: This change introduced different
matcher
to theassociatedPRs
graphql request mock and it leads to different errors based on the different matcher, state below....postOnce(/MATCHER_HERE/, { data: { repository: { commit123: { oid: "123", associatedPullRequests: { pageInfo: { endCursor: "NI", hasNextPage: false, }, nodes: [prs[0]], }, }, }, }, })
-
When matcher is plain old
"https://api.github.local/graphql"
(currently commited change); See current full error at https://github.com/semantic-release/github/actions/runs/10619388030/job/29436858061?pr=907#step:8:94 or see snippet below``` [test:integration] HttpError (RequestError) { [test:integration] cause: SyntaxError { [test:integration] message: '"undefined" is not valid JSON', [test:integration] }, [test:integration] request: { [test:integration] body: { [test:integration] body: 'Test release note body', [test:integration] draft: true, [test:integration] name: 'v1.0.0', [test:integration] prerelease: false, [test:integration] tag_name: 'v1.0.0', [test:integration] }, [test:integration] headers: { [test:integration] accept: 'application/vnd.github.v3+json', [test:integration] authorization: 'token [REDACTED]', [test:integration] 'content-type': 'application/json; charset=utf-8', [test:integration] 'user-agent': 'test octokit-core.js/6.1.2 Node.js/20.9.0 (win32; x64)', [test:integration] }, [test:integration] method: 'POST', [test:integration] request: { [test:integration] agent: undefined, [test:integration] fetch: Function fetchMockProxy { … }, [test:integration] hook: Function bound bound register {}, [test:integration] }, [test:integration] url: 'https://api.github.local/repos/test_user/test_repo/releases', [test:integration] }, [test:integration] ```
-
When matcher uses a function as seen below (this set-up similar to the matcher used in the mock request for
findSRIssue
uses digs into the request object an matches to intercepts request wherequery getAssociatedPRs
string is present in the query property)(_, { body }) => { t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/); return true; },
Error:
[test:integration] 685: .postOnce((_, { body }) => { [test:integration] 686: t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/); [test:integration] 687: return true; [test:integration] [test:integration] `t.regex()` must be called with a string [test:integration] [test:integration] Called with: [test:integration] [test:integration] undefined
-
When matcher uses same function and asserts the request url, seen below
(url, { body }) => { t.is(url, "https://api.github.local/graphql"); t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/); return true; }
Error:
[test:integration] 685: .postOnce((url, { body }) => { [test:integration] 686: t.is(url, "https://api.github.local/graphql"); [test:integration] 687: t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/); [test:integration] [test:integration] Difference (- actual, + expected): [test:integration] [test:integration] - 'https://api.github.local/repos/test_user/test_repo/releases' [test:integration] + 'https://api.github.local/graphql'
-
When matcher uses another supported function format, seen below
(_, __, { body }) => { t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/); return true; }
Error:
[test:integration] HttpError (RequestError) { [test:integration] cause: TypeError { [test:integration] message: 'Cannot destructure property \'body\' of \'undefined\' as it is undefined.', [test:integration] }, [test:integration] request: { [test:integration] body: { [test:integration] body: 'Test release note body', [test:integration] draft: true, [test:integration] name: 'v1.0.0', [test:integration] prerelease: false, [test:integration] tag_name: 'v1.0.0', [test:integration] }, [test:integration] headers: { [test:integration] accept: 'application/vnd.github.v3+json', [test:integration] authorization: 'token [REDACTED]', [test:integration] 'content-type': 'application/json; charset=utf-8', [test:integration] 'user-agent': 'test octokit-core.js/6.1.2 Node.js/20.9.0 (win32; x64)', [test:integration] }, [test:integration] method: 'POST', [test:integration] request: { [test:integration] agent: undefined, [test:integration] fetch: Function fetchMockProxy { … }, [test:integration] hook: Function bound bound register {}, [test:integration] }, [test:integration] url: 'https://api.github.local/repos/test_user/test_repo/releases', [test:integration] }, [test:integration] response: undefined, [test:integration] status: 500, [test:integration] message: 'Cannot destructure property \'body\' of \'undefined\' as it is undefined.', [test:integration] }
-
Important Note
Delete the test case "Verify, release and notify success"
itself 🫣 and every other 242 tests are passes.... I'm very convinced something is messed up in there that I need to figure out 😢
Yoooooo, I addressed it 🥳🥳🥳🥳 What I did 🤌Its important to note that in my function matchers, I implemented assertion as seen below.... (url, { body }) => {
t.is(url, "https://api.github.local/graphql");
t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/);
return true;
} ...well this works in some test cases and doesn't work in this particular troublesome case... But on a quick call with @gr2m today, I learnt I could/should just do away with assertions in the matchers... Hence I implement plain condition that return (url, { body }) =>
url === "https://api.github.local/graphql" &&
JSON.parse(body).query.includes("query getAssociatedPRs("), ...and voila, the bad boy works 😁. I was also experiencing similar troubles in #874 and all in now well 😉... I guess I can have a good weekend now 😃 |
This reverts commit a478a2b.
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.
Let's gooo! We discussed this extensively and @babblebey tested it locally, time to ship 🚀
🎉 This PR is included in version 10.3.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@babblebey the issue no longer fails my pipeline 👍, but why is it still using GH SearchAPI at all as a fallback (it eats up time in pipeline)? Shouldn't the new mechanism of finding issues be sufficient? In my case, no issue was found using GraphQL, so it still went to old logic as fallback? Thanks in advance!
|
Hey @zachspar, Great question. The SearchAPI was used to search "semantic-release fail issue" using just the The need for SearchAPI has fallback arises in cases when their are "semantic-release fail issue" created by the old setup (these are issues that didn't get the opinionated |
Gotcha, so it is for backward compatibility. Will there be any plans to deprecate the fallback logic? Or is there a mechanism it could use to search for these issues using GraphQL without labels? |
Says in there 😉 Lines 21 to 24 in b6ed55e
None at the moment, but if we do find any GraphQL means that works as simply as we used the SearchAPI with |
This Pull request replaces the usage of the troublesome 😉 GitHub SearchAPI as primary logic in the
findSRIssue
with graphql. It relegates the searchAPI consumption to a fallback logic that will be removed in a future release.The GraphQL functionality performs its search for Semantic Release Issue by querying for Issues on a repository with specific filter, in our case we use a
label
in the filter. This means that the PR introduces changes to the way we "create" and "find" Sematic Release issues in thefail
andsuccess
script.Changes Made:
Introduced an opinionated label for Semantic Release failing release issues in form of a constant
RELEASE_FAIL_LABEL
Concatenated the
RELEASE_FAIL_LABEL
to thelabel
param on creation of semantic release failing issue in thefail
script; this means the our opinionated label gets added to the failing release issue regardless of the plugincontext.label
configuration value (which is eithersemantic-release
,CUSTOM_LABEL
orfalse
)Introduced the GraphQL Query that the appropriated
filter
and passed into it is thelabels
filter withcontext.label
configuration concatenated with our opinionatedRELEASE_FAIL_LABEL
as value;Introduced error handling to the relegated GitHub searchAPI consumption; this allows for non-stop flow of step when an error occurs in the request to the hectic 😆 endpoint
Converted (as initially said) GitHub searchAPI consumption to fallback logic which triggers when no fail issue is found through newly established means of finding fail issues
Introduced the opinionated
RELEASE_FAIL_LABEL
to the failing release issues at creation as imperative label (no customizability)Related Issue
Fixes #867
Screencast
DEMO: Closing all Opened Semantic Release Issues (with
semantic-release
label,RELEASE_FAIL_LABEL
opinionated label and without any label)screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.7.mp4