Skip to content

Fix e2e notify script again #5911

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Fix e2e notify script again #5911

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f778306
Fix e2e notify script again
hsubox76 Jan 21, 2022
ff47fdb
Format
hsubox76 Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions scripts/ci/notify-test-result.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,14 @@ async function notifyTestResults() {
}

let message = `E2E Tests ${status}`;
let versionOrTag;

// Add version if it can find it in the workflow_dispatch event data.
if (process.env.GITHUB_EVENT_PATH) {
const wrPayload = require(process.env.GITHUB_EVENT_PATH);
if (wrPayload.inputs && wrPayload.inputs.versionOrTag) {
message += ` for release ${wrPayload.inputs.versionOrTag}.`;
versionOrTag = wrPayload.inputs.versionOrTag;
} else {
console.log(`Couldn't find versionOrTag in event payload.`);
}
Expand Down Expand Up @@ -102,13 +104,24 @@ async function notifyTestResults() {

req.on('error', error => reject(error));

req.write(
JSON.stringify({
testStatus,
testUrl: workflowUrl
}),
err => reject(err)
);
const data = {
testStatus,
testUrl: workflowUrl
};

if (versionOrTag) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a risk that versionOrTag is a truthy non-string? something like 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so because the workflow_dispatch input type defaults to string if you don't specify a type and I haven't here:

firebase-js-sdk/.github/workflows/e2e-test.yml

Line 7 in d612d6f

versionOrTag:

// Matches a staging version tag pattern.
const match = versionOrTag.match(/^(\d+.\d+.\d+)-\d+$/);
if (match) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like we should do a length check in case match has fewer than expected matches. Accessing 1 directly without validating that it's a valid key seems dangerous

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that outcome is possible, if the capturing group doesn't exist in the string, it won't match at all and match will be null. I think the only outcomes are null or an array of length 2. I tried a couple of test strings in the console and can't think of a case that would return non-null but also not have a captured group, let me know if I haven't thought of a case though, regex isn't my strength.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good I'm ok approving if you are confident

// Remove suffix from staging version
data.version = match[1];
// Full staging version with tag
data.tag = versionOrTag;
} else {
data.version = versionOrTag;
}
}
req.write(JSON.stringify(data), err => reject(err));
req.end();
});

Expand Down