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

Add tests for increased coverage. #4891

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Conversation

Lussebullen
Copy link
Contributor

- What I did

Increased test coverage of cli/compose/convert/volume.go to 100%.

Fixed typo in cli/compose/template/template_test.go to reach the intended branch test in extractVariables func
It checks for Strings.Contains func checks for :? and not ?:

Added tests of Descope and Name methods for Namespace in cli/compose/convert/compose_test.go.

Added test of convertEnvironment in case of nil value in cli/compose/convert/service_test.go.

Added tests for capitalizeFirst func in cli/command/utils_test.go.

Added tests of String and Type methods for NetworkOpt in opts/network_test.go.

Added test for GetAllOrEmpty method and for ParseCPUs in opts/opts_test.go.

- How I did it

We started by checking the Code coverage to see where testing was lacking and added test as we saw fit in order to increase the test coverage.

- How to verify it

Check difference as reported by codecov bot when triggered.

Verify it by running first running make dev and make test to check that all test passes and afterwards running the following commands to check that the test coverage has increaed:

$ go test -coverprofile=coverage.out ./...
$ go tool cover -html=coverage.out -o coverage.html

or

make test-coverage

- Description for the changelog

Add various test cases for increased test coverage.

- A picture of a cute animal (not mandatory but encouraged)
image

@Lussebullen
Copy link
Contributor Author

Lussebullen commented Feb 26, 2024

The failed tests here do not appear when running make test-coverage locally, so I cannot reproduce it at this time.

 55.07 === Failed
55.07 === FAIL: cli-plugins/socket TestConnectAndWait/connect_goroutine_exits_after_EOF (0.00s)
55.07     socket_test.go:111: assertion failed: 9 (int) != 10 (numGoroutines + 1 int)
55.07 
55.07 === FAIL: cli-plugins/socket TestConnectAndWait (0.00s)
55.07 
55.07 DONE 1719 tests, 1 skipped, 2 failures in 54.735s
------
Dockerfile:77
--------------------
  76 |     ENV GO111MODULE=auto
  77 | >>> RUN --mount=type=bind,target=.,rw \
  78 | >>>     --mount=type=cache,target=/root/.cache \
  79 | >>>     --mount=type=cache,target=/go/pkg/mod \
  80 | >>>     gotestsum -- -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')
  81 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c gotestsum -- -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')" did not complete successfully: exit code: 1
Error: buildx bake failed with: ERROR: failed to solve: process "/bin/sh -c gotestsum -- -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')" did not complete successfully: exit code: 1

I'm not entirely sure what to make of the error, but I see that it happens occasionally, e.g. here as well:
#4759. @thaJeztah do you know what to make of this?

@Benehiko
Copy link
Member

Benehiko commented Mar 8, 2024

The failed tests here do not appear when running make test-coverage locally, so I cannot reproduce it at this time.

 55.07 === Failed
55.07 === FAIL: cli-plugins/socket TestConnectAndWait/connect_goroutine_exits_after_EOF (0.00s)
55.07     socket_test.go:111: assertion failed: 9 (int) != 10 (numGoroutines + 1 int)
55.07 
55.07 === FAIL: cli-plugins/socket TestConnectAndWait (0.00s)
55.07 
55.07 DONE 1719 tests, 1 skipped, 2 failures in 54.735s
------
Dockerfile:77
--------------------
  76 |     ENV GO111MODULE=auto
  77 | >>> RUN --mount=type=bind,target=.,rw \
  78 | >>>     --mount=type=cache,target=/root/.cache \
  79 | >>>     --mount=type=cache,target=/go/pkg/mod \
  80 | >>>     gotestsum -- -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')
  81 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c gotestsum -- -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')" did not complete successfully: exit code: 1
Error: buildx bake failed with: ERROR: failed to solve: process "/bin/sh -c gotestsum -- -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')" did not complete successfully: exit code: 1

I'm not entirely sure what to make of the error, but I see that it happens occasionally, e.g. here as well: #4759. @thaJeztah do you know what to make of this?

@Lussebullen This is a flaky test unrelated to your changes and can be safely ignored :)

cli/compose/template/template_test.go Outdated Show resolved Hide resolved
cli/command/utils_test.go Outdated Show resolved Hide resolved
@Benehiko
Copy link
Member

Could you resolve the conflicts and the review suggestions? Other than that it looks good! :)

@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2024

Codecov Report

Merging #4891 (ccfd0b2) into master (ea3201c) will increase coverage by 0.26%.
Report is 12 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4891      +/-   ##
==========================================
+ Coverage   61.44%   61.70%   +0.26%     
==========================================
  Files         289      289              
  Lines       20241    20241              
==========================================
+ Hits        12437    12490      +53     
+ Misses       6903     6864      -39     
+ Partials      901      887      -14     

@Lussebullen
Copy link
Contributor Author

Lussebullen commented Mar 17, 2024

I force pushed an amend with my signature to fix the DCO issue, but I'm not sure I can trigger the tests to run again.
I fixed the table test as requested and made a comment on the ?: vs :? section :)

@Benehiko
Copy link
Member

Thank you @Lussebullen, there is still a merge conflict with cli/command/utils_test.go. Would you be able to update your branch and fix the merge conflicts?

@Lussebullen
Copy link
Contributor Author

@Benehiko reverted the ?: / :? part as you requested, I also updated the branch, but in doing so it seems the test capitalizeFirst utility no longer builds, for some reason it no longer exports despite doing so before. So I removed it entirely.

Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

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

Thanks @Lussebullen, LGTM :)

@Lussebullen
Copy link
Contributor Author

My pleasure 🫡
Have a good one ✌️

Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM; most of these could be refactored into a table-based tests (thinking of the convertVolumeToMount tests).

But this is still an improvement and could be a follow up 😄

Thanks!

@Benehiko
Copy link
Member

Hey @Lussebullen sorry to ask more of you 🙈, but could you squash the commits you have on this branch into a single commit so we can merge this?

Co-authored-by: Adam Siraj <[email protected]>
Co-authored-by: Emil Sjölander <[email protected]>
Co-authored-by: Omar Askar Vergara <[email protected]>
Co-authored-by: Emir Catir <[email protected]>

Signed-off-by: Mathias Duedahl <[email protected]>
@Lussebullen
Copy link
Contributor Author

@Benehiko I squashed them now, I just added my co-authors in the commit message, not sure how it works with the ctn, but its important to me that they get the credit they are due :)

@Benehiko Benehiko added this to the 27.0.0 milestone Mar 21, 2024
@Benehiko Benehiko merged commit 4468148 into docker:master Mar 21, 2024
88 checks passed
@vvoland vvoland modified the milestones: 27.0.0, 26.1.0 Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants