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.

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

Jump to bottom

tests: add more coverage to composeStatus when response is InternalServerError #1306

Merged
merged 1 commit into from
Aug 27, 2024
Merged

tests: add more coverage to composeStatus when response is InternalServerError #1306

merged 1 commit into from
Aug 27, 2024

Conversation

mgold1234
Copy link
Contributor

This commit add unit test for getComposeStatus handling the StatusInternalServerError response after composeStatus found.
Increase the coverage of GetComposeStatus from 70.3% to 83.8%

@lzap lzap force-pushed the add_more_coverage_to_getCopose_notStatusOk branch from 8dea797 to 0e6453e Compare August 20, 2024 10:13
lzap
lzap previously approved these changes Aug 20, 2024
View reviewed changes
@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch from 0e6453e to ca983d4 Compare August 22, 2024 09:39
schuellerf
schuellerf requested changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

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

Raising the coverage is super cool 🥳📈
as this test is pretty similar to TestGetComposeStatusNotFoundResponse maybe you can use the pattern testCases := []struct {… and for _, tc := range testCases { as implemented for example in TestWithoutOsbuildComposerBackend or similar here too and unify
TestGetComposeStatusNotFoundResponse with TestGetComposeStatusInternalServerErrorResponse in one test function (having all common code only once

internal/v1/handler_test.go Outdated Show resolved Hide resolved
@mgold1234
Copy link
Contributor Author

@schuellerf in TestGetComposeStatusNotFoundResponse
I have in the apiSrv w.WriteHeader(http.StatusNotFound)
and in TestGetComposeStatusInternalServerErrorResponse I have in the apiSrv w.WriteHeader(http.StatusInternalServerError) I am not sure I can handle that the same way as TestGetComposeStatusNotFoundResponse

@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch from ca983d4 to 3940e4f Compare August 22, 2024 11:17
schuellerf
schuellerf previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

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

You can rewrite to make less code duplication or we'll take it as is for now, so I'll approve and you can choose

internal/v1/handler_test.go Show resolved Hide resolved
@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch from 3940e4f to ae058e4 Compare August 22, 2024 13:40
ezr-ondrej
ezr-ondrej previously approved these changes Aug 22, 2024
View reviewed changes
@ezr-ondrej
Copy link
Collaborator

The unit tests are failing now, apart of that, nice job! 🎉

@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch 2 times, most recently from 04f488a to 9796545 Compare August 22, 2024 14:06
@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch 2 times, most recently from 5bdd168 to ea30952 Compare August 26, 2024 08:09
schuellerf
schuellerf requested changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

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

so I guess TestGetComposeStatusNotFoundResponse() is not obsolete and can be removed?
Looks nice once the code duplication is removed!

@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch from ea30952 to f7ee2dc Compare August 26, 2024 09:01
schuellerf
schuellerf approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch from f7ee2dc to 07c50ea Compare August 26, 2024 18:17
@mgold1234
tests: add more coverage to composeStatus when response is InternalSe…
2034e1d 
…rverError

This commit add unit test for getComposeStatus handling the StatusInternalServerError
response after composeStatus found.
Increase the coverage of GetComposeStatus from 70.3% to 83.8%
@mgold1234 mgold1234 force-pushed the add_more_coverage_to_getCopose_notStatusOk branch from 07c50ea to 2034e1d Compare August 27, 2024 07:37
@mgold1234 mgold1234 requested a review from lzap August 27, 2024 07:37
@ezr-ondrej ezr-ondrej merged commit 64e339d into osbuild:main Aug 27, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schuellerf schuellerf schuellerf approved these changes

@ezr-ondrej ezr-ondrej Awaiting requested review from ezr-ondrej

@lzap lzap Awaiting requested review from lzap

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mgold1234 @ezr-ondrej @lzap @schuellerf