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

action: update workflow to block low unit test coverage code #460

Merged
merged 1 commit into from
May 6, 2023

Conversation

adamqqqplay
Copy link
Contributor

In order to improve the code quality of this project, we have introduced a test coverage check in CI to prevent code with low unit test coverage from being merged. Although there is no perfect code coverage value, we can still make a stab at such a value. 3 years ago, Google posted a blog post on their best practices:

We offer the general guidelines of 60% as “acceptable”, 75% as “commendable” and 90% as “exemplary.”

Therefore, in order to increase the overall code coverage of this project to 60% mentioned in #334, we set the coverage of each patch to 75% and above.

Patch details:

  1. The project's unit test coverage cannot be reduced by more than 0.5%.
  2. The unit test coverage of each patch must be higher than 75%.

Reference: https://about.codecov.io/blog/the-case-against-100-code-coverage/

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2023

Codecov Report

Merging #460 (689c25c) into main (f89d069) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #460   +/-   ##
=======================================
  Coverage   37.13%   37.13%           
=======================================
  Files          60       60           
  Lines        6912     6912           
=======================================
  Hits         2567     2567           
  Misses       4048     4048           
  Partials      297      297           

wait_for_ci: false

# When modifying this file, please validate using
# curl -X POST --data-binary @codecov.yml https://codecov.io/validate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a blank line here, others LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

1. The project's unit test coverage cannot be reduced by more than 0.5%.
2. The unit test coverage of each patch must be higher than 75%.

Signed-off-by: Qinqi Qu <[email protected]>
@adamqqqplay
Copy link
Contributor Author

Smoke tests usually fail and may need to be re-run.

@changweige
Copy link
Member

@imeoer Should we bump the nydus-image for Converter UT tests?

time="2023-04-28T03:24:51Z" level=info module=builder
time="2023-04-28T03:24:51Z" level=info msg="USAGE:" module=builder
time="2023-04-28T03:24:51Z" level=info msg=" nydus-image unpack --bootstrap --log-level --output " module=builder
time="2023-04-28T03:24:51Z" level=info module=builder
time="2023-04-28T03:24:51Z" level=info msg="For more information try --help" module=builder
time="2023-04-28T03:24:51Z" level=error msg="fail to run /usr/bin/nydus-image [unpack --log-level warn --bootstrap /tmp/nydus-converter-1649642338/image.boot --output /tmp/nydus-converter-1649642338/oci.tar --backend-config /tmp/nydus-converter-1649642338/backend-config.json]" error="exit status 1"
converter_test.go:303:
Error Trace: /home/runner/work/nydus-snapshotter/nydus-snapshotter/tests/converter_test.go:303
/home/runner/work/nydus-snapshotter/nydus-snapshotter/tests/converter_test.go:589
/home/runner/work/nydus-snapshotter/nydus-snapshotter/tests/converter_test.go:566
Error: Received unexpected error:
exit status 1
unpack
github.com/containerd/nydus-snapshotter/pkg/converter.Unpack
/home/runner/work/nydus-snapshotter/nydus-snapshotter/pkg/converter/convert_unix.go:675
github.com/containerd/nydus-snapshotter/tests.unpackLayer
/home/runner/work/nydus-snapshotter/nydus-snapshotter/tests/converter_test.go:300
github.com/containerd/nydus-snapshotter/tests.testUnpack
/home/runner/work/nydus-snapshotter/nydus-snapshotter/tests/converter_test.go:589
github.com/containerd/nydus-snapshotter/tests.TestUnpack
/home/runner/work/nydus-snapshotter/nydus-snapshotter/tests/converter_test.go:566
testing.tRunner
/opt/hostedtoolcache/go/1.19.6/x64/src/testing/testing.go:1446
runtime.goexit
/opt/hostedtoolcache/go/1.19.6/x64/src/runtime/asm_amd64.s:1594
Test: TestUnpack
--- FAIL: TestUnpack (0.14s)
=== RUN TestImageConvert
time="2023-04-28T03:25:25Z"

@changweige
Copy link
Member

Could you please share a page showing this newly added coverage action results?

@adamqqqplay
Copy link
Contributor Author

Could you please share a page showing this newly added coverage action results?

@changweige This PR does not add a new action, it just adds a coverage check to the existing action, you can refer to following links. They are a check result of this PR: dragonflyoss/nydus#1225

  1. https://github.com/dragonflyoss/image-service/runs/12950553738
  2. https://github.com/dragonflyoss/image-service/runs/12950553733

image

@changweige changweige merged commit 2455464 into containerd:main May 6, 2023
@adamqqqplay adamqqqplay deleted the update-test-coverage branch May 8, 2023 08:43
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.

4 participants