magefile: added combined test coverage support for `mage test:all` #2235

kartikaysaxena · 2025-02-04T21:19:53Z

Fixes #1348 mage test:all now generates combined coverprofile aggregated into coverage.txt

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

josephschorr · 2025-02-05T17:00:01Z

magefiles/util.go

+	testArgs := append([]string{
+		"test",
+		"-covermode=atomic",
+		"-coverprofile=coverage.txt",


Won't this overwrite the coverage.txt file when each individual test suite runs? Can you try running mage test:all and ensure it combines the coverage?

Changed the approach, now each test writes to a different file and removes those files in the end, go tool cover -html=coverage.txt now shows

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

josephschorr · 2025-02-06T20:27:21Z

magefiles/util.go

@@ -210,3 +223,45 @@ func run(dir string, env map[string]string, stdout, stderr io.Writer, cmd string
 	err = c.Run()
 	return sh.CmdRan(err), sh.ExitStatus(err), err
 }
+
+func sanitizePath(path string) string {


I'd recommend hashing the path here

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

vroldanbet

Seems like the PR enables coverage by default on all, whereas before it was opt in. Do we have concerns that would add overhead on the inner loop? Or is the overhead negligible?

Signed-off-by: Kartikay <120778728+kartikaysaxena@users.noreply.github.com>

kartikaysaxena · 2025-02-10T22:41:41Z

Seems like the PR enables coverage by default on all, whereas before it was opt in. Do we have concerns that would add overhead on the inner loop? Or is the overhead negligible?

I used time mage test:all to check and the overhead was costing over a minute, followed the pattern for other targets for generating coverage reports which was earlier in UnitCover, now we have a dedicated test:allcover for this, is this correct? Would love the feedback and any improvements further if necessary

vroldanbet · 2025-02-11T18:39:46Z

magefiles/util.go

+	}
+	defer f.Close()
+
+	args := []string{"run", "github.com/wadey/gocovmerge@latest"}


Please use go tool covdata merge, see https://go.dev/doc/build-cover

Edit: it seems that go tool covdata merge works for only binary coverage files like covmeta.* and covcounters.* and not for text-based cover files

vroldanbet · 2025-02-11T18:41:00Z

magefiles/util.go

@@ -210,3 +221,40 @@ func run(dir string, env map[string]string, stdout, stderr io.Writer, cmd string
 	err = c.Run()
 	return sh.CmdRan(err), sh.ExitStatus(err), err
 }
+
+func hashPath(path string) string {


Do we need to hash the path? I'd keep it simpler by generating a UUID as suffix

vroldanbet · 2025-02-11T18:48:04Z

magefiles/test.go

+func (t Test) AllCover() error {
+	ds := Testds{}
+	c := Testcons{}
+	mg.Deps(t.UnitCover, t.IntegrationCover, t.SteelthreadCover, t.ImageCover, t.AnalyzersCover,


this seems like a lot of duplication that would lead to drift w.r.t to All() method. Ideally All and AllCover invoke `all(coverage bool)

vroldanbet · 2025-02-11T18:48:31Z

magefiles/util.go

+		return err
+	}
+
+	for _, file := range files {


Why deleting the tests?

was for deleting individual coverage files after merging them. reverting

vroldanbet · 2025-02-11T18:50:27Z

magefiles/test.go

-		"-tags", "ci,docker,datastoreconsistency",
-		"-timeout", "10m",
-		"-run", fmt.Sprintf("TestConsistencyPerDatastore/%s", datastore))
+	if coverage {


all this if coverage blocks could be refactored to avoid all the duplication - e.g. a function that takes the args and the coverage bool, and returns the args with the appended parameters.

vroldanbet · 2025-02-11T18:51:31Z

magefiles/test.go

+
+func (Testds) spanner(coverage bool) error {
+	args := []string{"-tags", "ci,docker", "-timeout", "10m"}
+	if coverage {


lots of duplication: this coverage if statement should be handled inside datastoreTest

vroldanbet · 2025-02-11T18:52:57Z

magefiles/test.go

+func (Test) image(coverage bool) error {
+	mg.Deps(Build{}.Testimage)
+	args := []string{"-tags", "docker,image"}
+	if coverage {


this should be moved into goDirTest, otherwise it leads to a lot of duplication

vroldanbet · 2025-02-11T18:53:21Z

magefiles/test.go

@@ -43,27 +53,75 @@ func (Test) unit(coverage bool) error {
 	fmt.Println("running unit tests")
 	args := []string{"-tags", "ci,skipintegrationtests", "-race", "-timeout", "10m", "-count=1"}
 	if coverage {


this should be moved into goTest, otherwise it leads to a lot of duplication

vroldanbet · 2025-02-11T18:55:21Z

magefiles/test.go

+}
+
+// SteelthreadCover Runs the steelthread tests and generates a coverage report
+func (t Test) SteelthreadCover() error {


It's annoying we end up with proliferation of commands just to have with/without coverage versions of them. Calling mage leads to a very long list that starts to become not very user friendly.

I know mage supports arguments in commands, but not if it supports commands with arguments and default values, could you investigate?

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena · 2025-02-13T00:29:31Z

Utilised uuid instead, also passed the bool through a context to goDirTest and goDirTestWithEnv, now works like mage test:all -cover=true

added test coverage

80503bb

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena requested a review from a team as a code owner February 4, 2025 21:19

josephschorr reviewed Feb 5, 2025

View reviewed changes

different profiles for tests

64f1377

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena force-pushed the mage-test-coverage branch from 46942c5 to 64f1377 Compare February 5, 2025 19:07

kartikaysaxena requested a review from josephschorr February 5, 2025 19:13

josephschorr reviewed Feb 6, 2025

View reviewed changes

kartikaysaxena and others added 2 commits February 8, 2025 18:32

Merge branch 'main' into mage-test-coverage

3c14817

use path hash instead

59fd80d

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena force-pushed the mage-test-coverage branch from ba7d1da to 59fd80d Compare February 8, 2025 13:42

vroldanbet reviewed Feb 10, 2025

View reviewed changes

kartikaysaxena force-pushed the mage-test-coverage branch 2 times, most recently from 4fb47db to 7498752 Compare February 10, 2025 22:12

separate targets for cover

3f62b00

Signed-off-by: Kartikay <120778728+kartikaysaxena@users.noreply.github.com>

kartikaysaxena force-pushed the mage-test-coverage branch from 7498752 to 3f62b00 Compare February 10, 2025 22:36

kartikaysaxena requested a review from vroldanbet February 11, 2025 12:33

vroldanbet reviewed Feb 11, 2025

View reviewed changes

context coverage bool

752a5ea

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena requested a review from vroldanbet February 13, 2025 00:29

Merge branch 'main' into mage-test-coverage

e7466ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

magefile: added combined test coverage support for `mage test:all` #2235

magefile: added combined test coverage support for `mage test:all` #2235

kartikaysaxena commented Feb 4, 2025

josephschorr Feb 5, 2025

kartikaysaxena Feb 5, 2025

josephschorr Feb 6, 2025

kartikaysaxena Feb 8, 2025

vroldanbet left a comment

kartikaysaxena commented Feb 10, 2025

vroldanbet Feb 11, 2025

kartikaysaxena Feb 13, 2025 •

edited

Loading

vroldanbet Feb 11, 2025

vroldanbet Feb 11, 2025

vroldanbet Feb 11, 2025

kartikaysaxena Feb 13, 2025

vroldanbet Feb 11, 2025

vroldanbet Feb 11, 2025

vroldanbet Feb 11, 2025

vroldanbet Feb 11, 2025

vroldanbet Feb 11, 2025

kartikaysaxena commented Feb 13, 2025

magefile: added combined test coverage support for mage test:all #2235

Are you sure you want to change the base?

magefile: added combined test coverage support for mage test:all #2235

Conversation

kartikaysaxena commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vroldanbet left a comment

Choose a reason for hiding this comment

kartikaysaxena commented Feb 10, 2025

Choose a reason for hiding this comment

kartikaysaxena Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikaysaxena commented Feb 13, 2025

magefile: added combined test coverage support for `mage test:all` #2235

magefile: added combined test coverage support for `mage test:all` #2235

kartikaysaxena Feb 13, 2025 •

edited

Loading