Add unit test coverage requirements for some packages #22401

lunny · 2023-01-11T09:16:15Z

This PR add a CI check for package tests coverage. When the coverage is less than a previous value, CI will be failure.

KN4CK3R · 2023-01-11T15:10:28Z

I think this should use the merged results. Currently it only uses the integration test result.

lunny · 2023-01-12T08:24:30Z

I think this should use the merged results. Currently it only uses the integration test result.

Done

Makefile

KN4CK3R · 2023-01-16T09:28:36Z

2023/01/16 09:02:54 package code.gitea.io/gitea/modules/validation coverage is 79.4%, required 80.1%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/webhook coverage is 13.3%, required 13.7%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/svg coverage is 94.4%, required 96.1%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/doctor coverage is 15.1%, required 20.8%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/cache coverage is 14.5%, required 14.6%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/emoji coverage is 83.3%, required 85.1%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/auth coverage is NaN%, required 60.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/graceful coverage is 29.0%, required 29.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/indexer coverage is NaN%, required 39.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/services/cron coverage is 56.2%, required 61.4%
2023/01/16 09:02:54 package code.gitea.io/gitea/services/forms coverage is 36.5%, required 36.8%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/proxy coverage is 17.2%, required 21.4%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/process coverage is 38.5%, required 40.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/services/convert coverage is 73.5%, required 74.8%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/issue coverage is NaN%, required 75.4%
2023/01/16 09:02:54 package code.gitea.io/gitea/routers/api coverage is NaN%, required 60.6%
2023/01/16 09:02:54 package code.gitea.io/gitea/routers/install coverage is 5.7%, required 7.3%

Now we see that it works. But it looks like our code calculates the coverage different than CodeCov.

lunny · 2023-01-16T09:30:25Z

2023/01/16 09:02:54 package code.gitea.io/gitea/modules/validation coverage is 79.4%, required 80.1%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/webhook coverage is 13.3%, required 13.7%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/svg coverage is 94.4%, required 96.1%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/doctor coverage is 15.1%, required 20.8%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/cache coverage is 14.5%, required 14.6%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/emoji coverage is 83.3%, required 85.1%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/auth coverage is NaN%, required 60.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/graceful coverage is 29.0%, required 29.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/indexer coverage is NaN%, required 39.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/services/cron coverage is 56.2%, required 61.4%
2023/01/16 09:02:54 package code.gitea.io/gitea/services/forms coverage is 36.5%, required 36.8%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/proxy coverage is 17.2%, required 21.4%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/process coverage is 38.5%, required 40.2%
2023/01/16 09:02:54 package code.gitea.io/gitea/services/convert coverage is 73.5%, required 74.8%
2023/01/16 09:02:54 package code.gitea.io/gitea/modules/issue coverage is NaN%, required 75.4%
2023/01/16 09:02:54 package code.gitea.io/gitea/routers/api coverage is NaN%, required 60.6%
2023/01/16 09:02:54 package code.gitea.io/gitea/routers/install coverage is 5.7%, required 7.3%

Now we see that it works. But it looks like our code calculates the coverage different than CodeCov.

yes, For those slight differences, I think it's accepted.

KN4CK3R · 2023-01-16T09:49:52Z

Looks like CodeCov combines the results of the subdirectories. Our calculation displays the result for the files in that directory only (?). That would explain the NaN% lines.

6543 · 2023-04-02T22:24:45Z

I would lower the current requirements to match what we have atm ... and then do increase them by case

Makefile

silverwind · 2023-04-05T19:20:29Z

When the coverage is less than a previous value, CI will be failure

I forsee some PR friction if we add this. I'd guess more PRs than usual would become abandoned with such strict requirements as not every bug fix is easily testable. We can try, but I guess there should be a mechanism for a owner to overrule this requirement for individual PRs.

wxiaoguang · 2023-05-01T07:50:59Z

Is it the right way to go?

6543 · 2023-05-02T17:08:37Z

I think in worst case we can lower the number for a fix in that pull ... but we should enforce it ... we want good tested code

wxiaoguang · 2023-05-02T17:11:59Z

I agree to improve test code (when I review, I frequently suggest to add tests ...)

But, is it the right way to go? Filling the Makefile with a lot of percentage numbers?

6543 · 2023-05-02T17:18:05Z

we could improve the tool that checks it and add a new .min_coverage.conf or something like that witch do contain the threashhold

wxiaoguang · 2023-05-02T17:38:57Z

A clear Makefile could L-G-T-M

.github/workflows/pull-db-tests.yml

.drone.yml

.github/workflows/pull-db-tests.yml

wxiaoguang · 2023-06-14T12:30:52Z

build/gocoverage.go

+	if err != nil {
+		log.Fatalf("invalid percent: %s", percent)
+	}
+	return int64(i * 10)


Why i*10 ? 12.34% means "123"? That's a strange unit.

Some output will be 12.3%, and I think it's accurate enough.

I would say this trick is difficult to understand, and I do not understand why it must be casted to int64

IMO keep the original float "12.3" is clear enough, or unify the unit to natural unit (1.0 mean 100%, 0.02 mean 2%)

build/gocoverage.go

.github/workflows/pull-db-tests.yml

Makefile

silverwind · 2023-06-14T17:09:14Z

Will there be an option to ignore reduced coverage? Like when some stuff is hard/impossible to test for, e.g. admin should still be able to merge.

Generally, I'm -0 on this topic, as it may hinder contribution.

silverwind · 2023-06-17T07:32:21Z

.github/workflows/pull-db-tests.yml

+        go-version: ">=1.20"
+        check-latest: true
+    - uses: actions/download-artifact@v3
+    - run: go run build/gocoverage.go merge unit-test-coverage/coverage.out unit-test-coverage-gogit/coverage.out unit-test-coverage-integration/coverage.out > coverage.all


Calls like this should be in the Makefile so that coverage can be produced locally as well. Please don't lock us in into actions with commands like this.

lunny · 2023-07-14T08:00:47Z

Will there be an option to ignore reduced coverage? Like when some stuff is hard/impossible to test for, e.g. admin should still be able to merge.

Generally, I'm -0 on this topic, as it may hinder contribution.

Looks like we need to ask contributors to provide tests case by case. But should always encourage tests in PR.
So I will close this one.

silverwind · 2023-07-14T10:57:09Z

Yes, I'm always asking where possible, I think it's the best we can do. Strict requirements will not work so good I guess.

lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Jan 11, 2023

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 11, 2023

KN4CK3R reviewed Jan 15, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

6543 approved these changes Apr 2, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 2, 2023

6543 added this to the 1.20.0 milestone Apr 2, 2023

silverwind reviewed Apr 5, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

lunny added the pr/wip This PR is not ready for review label May 1, 2023

wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label May 2, 2023

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2023

delvh removed this from the 1.20.0 milestone Jun 7, 2023

pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2023

silverwind reviewed Jun 13, 2023

View reviewed changes

.github/workflows/pull-db-tests.yml Outdated Show resolved Hide resolved

6543 reviewed Jun 13, 2023

View reviewed changes

.drone.yml Outdated Show resolved Hide resolved

lunny added 2 commits June 14, 2023 19:28

Add unit test coverage requirements for some packages

85ea22e

check all tests coverage but not only unit test

3ac48ce

lunny added 5 commits June 14, 2023 19:29

add more package test requirement

b2db338

ignore accurate coverage percent

56802c0

Use artifact to upload coverages and download then when merging

164d427

take .drone.yml back

9803449

Fix test

495eb4a

lunny force-pushed the lunny/add_package_unittest_requirement branch from 7ffa15c to 495eb4a Compare June 14, 2023 11:46

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 14, 2023

wxiaoguang reviewed Jun 14, 2023

View reviewed changes

.github/workflows/pull-db-tests.yml Outdated Show resolved Hide resolved

wxiaoguang reviewed Jun 14, 2023

View reviewed changes

build/gocoverage.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Jun 14, 2023

View reviewed changes

build/gocoverage.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Jun 14, 2023

View reviewed changes

build/gocoverage.go Outdated Show resolved Hide resolved

silverwind reviewed Jun 14, 2023

View reviewed changes

.github/workflows/pull-db-tests.yml Outdated Show resolved Hide resolved

silverwind reviewed Jun 14, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

lunny added 5 commits June 15, 2023 10:31

Follow silverwind and wxiaoguang's suggestion

3901862

Merge branch 'main' into lunny/add_package_unittest_requirement

6e70b13

Fix check

cbad492

Merge branch 'main' into lunny/add_package_unittest_requirement

68b8fab

Fix test

d9b221e

silverwind reviewed Jun 17, 2023

View reviewed changes

Merge branch 'main' into lunny/add_package_unittest_requirement

07565b5

lunny closed this Jul 14, 2023

lunny deleted the lunny/add_package_unittest_requirement branch July 14, 2023 08:00

go-gitea locked as resolved and limited conversation to collaborators Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test coverage requirements for some packages #22401

Add unit test coverage requirements for some packages #22401

lunny commented Jan 11, 2023 •

edited

Loading

KN4CK3R commented Jan 11, 2023

lunny commented Jan 12, 2023

KN4CK3R commented Jan 16, 2023 •

edited

Loading

lunny commented Jan 16, 2023

KN4CK3R commented Jan 16, 2023

6543 commented Apr 2, 2023

silverwind commented Apr 5, 2023 •

edited

Loading

wxiaoguang commented May 1, 2023

6543 commented May 2, 2023 •

edited

Loading

wxiaoguang commented May 2, 2023

6543 commented May 2, 2023

wxiaoguang commented May 2, 2023

wxiaoguang Jun 14, 2023

lunny Jun 15, 2023

wxiaoguang Jun 15, 2023 •

edited

Loading

silverwind commented Jun 14, 2023 •

edited

Loading

silverwind Jun 17, 2023 •

edited

Loading

lunny commented Jul 14, 2023

silverwind commented Jul 14, 2023

Add unit test coverage requirements for some packages #22401

Add unit test coverage requirements for some packages #22401

Conversation

lunny commented Jan 11, 2023 • edited Loading

KN4CK3R commented Jan 11, 2023

lunny commented Jan 12, 2023

KN4CK3R commented Jan 16, 2023 • edited Loading

lunny commented Jan 16, 2023

KN4CK3R commented Jan 16, 2023

6543 commented Apr 2, 2023

silverwind commented Apr 5, 2023 • edited Loading

wxiaoguang commented May 1, 2023

6543 commented May 2, 2023 • edited Loading

wxiaoguang commented May 2, 2023

6543 commented May 2, 2023

wxiaoguang commented May 2, 2023

wxiaoguang Jun 14, 2023

Choose a reason for hiding this comment

lunny Jun 15, 2023

Choose a reason for hiding this comment

wxiaoguang Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

silverwind commented Jun 14, 2023 • edited Loading

silverwind Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

lunny commented Jul 14, 2023

silverwind commented Jul 14, 2023

lunny commented Jan 11, 2023 •

edited

Loading

KN4CK3R commented Jan 16, 2023 •

edited

Loading

silverwind commented Apr 5, 2023 •

edited

Loading

6543 commented May 2, 2023 •

edited

Loading

wxiaoguang Jun 15, 2023 •

edited

Loading

silverwind commented Jun 14, 2023 •

edited

Loading

silverwind Jun 17, 2023 •

edited

Loading