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

test(ci): add tests for release process #241

Merged
merged 4 commits into from
Mar 10, 2022
Merged

test(ci): add tests for release process #241

merged 4 commits into from
Mar 10, 2022

Conversation

eunjae-lee
Copy link
Contributor

🧭 What and Why

Changes included:

  • Extract code as methods, and add tests for release process

I'm going to change the release process a bit, and need some tests to make sure the changes won't fail.

🧪 Test

  • CI

@netlify
Copy link

netlify bot commented Mar 9, 2022

✔️ Deploy Preview for api-clients-automation canceled.

🔨 Explore the source changes: d68ccee

🔍 Inspect the deploy log: https://app.netlify.com/sites/api-clients-automation/deploys/622a0f3a489db20008a4d947

@eunjae-lee eunjae-lee requested review from a team, damcou, millotp and shortcuts and removed request for a team and millotp March 9, 2022 09:51
damcou
damcou previously approved these changes Mar 9, 2022
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Look fine on my side, but maybe it would be better to have a double-check on the typescript

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

this looks good! small comments

Comment on lines +18 to +28
it('returns error when language scope is missing', () => {
expect(parseCommit(`abcdefg fix: fix the thing`)).toEqual({
error: 'missing-language-scope',
});
});

it('returns error when language scope is unknown', () => {
expect(parseCommit(`abcdefg fix(basic): fix the thing`)).toEqual({
error: 'unknown-language-scope',
});
});
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be a throw or we handle this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returns error and prints some warning logs.

https://github.com/algolia/api-clients-automation/blob/d68ccee283af203e0e562c1cfb6ed0064b80b2d9/scripts/release/create-release-issue.ts#L204:L213

Assuming most of the commits follow semantic rules, some of commits might slip through (like "Merge ...."), and it shouldn't fail the release process. We can just ignore those commits and exclude from the changelogs, and versioning process.

Copy link
Member

Choose a reason for hiding this comment

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

Yep that make sense, but I believe it would also make sense to have an error log in that case (maybe a file like yarn does for example, or a commit, or msg to slack).

Otherwise, we might not see the error and have release with missing commits, it might be intended to skip that commit in particular but it could also be a mistake on our side.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I'm dumb it's in the issue then ._.

Copy link
Contributor Author

@eunjae-lee eunjae-lee Mar 10, 2022

Choose a reason for hiding this comment

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

Oh, it's not in the issue, but we should definitely include them in the issue, like

Expand to see skipped commit messages:

lack of language scope

abcdefg fix: fix this

unknown language scope

abcdefg fix(blah): fix this

I can do this in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yep it make sense then, we definitely need to track those

@eunjae-lee eunjae-lee requested a review from shortcuts March 10, 2022 14:52
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Good to go! thanks for the tests :D

(Don't forget to add the task to track no-languages commits)

@eunjae-lee eunjae-lee merged commit 853dd99 into main Mar 10, 2022
@eunjae-lee eunjae-lee deleted the test/release branch March 10, 2022 15:10
shortcuts pushed a commit that referenced this pull request Apr 22, 2022
* test(ci): add tests for release process

* chore: update test case

* chore: update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants