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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

magefiles/util.go

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 <[email protected]>

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

vroldanbet · 2025-02-18T11:10:45Z

magefiles/util.go

+	testArgs := append([]string{
+		"test",
+		"-failfast",
+		"-count=1",
+	}, args...)
+	if cover, _ := ctx.Value("cover").(bool); cover {
+		if err := os.MkdirAll("coverage", 0o755); err != nil {
+			return fmt.Errorf("failed to create coverage directory: %w", err)
+		}
+		testArgs = append(testArgs, []string{
+			"-covermode=atomic",
+			fmt.Sprintf("-coverprofile=coverage-%s.txt", uuid.New().String()),
+		}...)
+	}


can you please refactor this logic into a single method that is used in both goDirTestWithEnv and goDirTest?

done with testWithArgs

vroldanbet · 2025-02-18T11:11:27Z

magefiles/util.go

+		"-failfast",
+		"-count=1",
+	}, args...)
+	if cover, _ := ctx.Value("cover").(bool); cover {


nit: would you mind replacing this with https://github.com/authzed/ctxkey 🙏🏻

vroldanbet · 2025-02-18T11:12:53Z

magefiles/util.go

+		"-count=1",
+	}, args...)
+	if cover, _ := ctx.Value("cover").(bool); cover {
+		if err := os.MkdirAll("coverage", 0o755); err != nil {


Do we need to give permissions to group and public? I think the owner having access is enough

vroldanbet · 2025-02-18T11:18:06Z

magefiles/test.go

+	ctx := context.Background()
+	cover := false
+	for _, arg := range os.Args {
+		if arg == "-cover=true" {
+			cover = true
+			break
+		}
+	}
+	ctx = context.WithValue(ctx, "cover", cover)


Using the context to propagate arguments is a good idea to workaround mage parameterization limitations 👍🏻

Can you refactor this into a function in case we need to add more flags for other commands? I also suggest using https://github.com/authzed/ctxkey for type-safe context values

vroldanbet · 2025-02-18T11:19:24Z

magefiles/test.go

 	return nil
 }

+func (t Test) Combine() error {


Is combining everything something that has to be invoked explicitly? Could we call this from All if the cover flag was enabled?

Signed-off-by: Kartikay <[email protected]>

tstirrat15

See comment; once that's through it looks good to me.

tstirrat15 · 2025-02-26T19:57:22Z

magefiles/test.go

+	ctxMyKey := ctxkey.New[bool]()
+	ctx := ctxMyKey.WithValue(context.Background(), cover)


Does it work to declare ctxMyKey twice? I'd expect that that would give you two different identities of keys, which wouldn't allow you to pull values out of context. The idea is that you'd give the key a name that means something and declare it once at the top of the file. There's an example in the ctxkey examples dir.

Would you please declare this key once at the top of the module with a meaningful name and then reuse it?

was doing that to avoid a global var, seems like it should be the way to go for this.

vroldanbet · 2025-02-27T12:05:05Z

magefiles/test.go

@@ -19,51 +22,56 @@ var emptyEnv map[string]string
 func (t Test) All() error {
 	ds := Testds{}
 	c := Testcons{}
-	mg.Deps(t.Unit, t.Integration, t.Steelthread, t.Image, t.Analyzers,
+	cover := parseCommandLineFlags()
+	ctxMyKey := ctxkey.New[bool]()


This should be defined as a global var, a key to a specific value.
Also you could add context.Context to All signature, mage supports it

vroldanbet · 2025-02-27T12:06:15Z

magefiles/util.go

+		"-failfast",
+		"-count=1",
+	}, args...)
+	ctxMyKey := ctxkey.New[bool]()


this should reference the global var

…a/spicedb into mage-test-coverage

magefiles/util.go

vroldanbet · 2025-02-28T09:35:48Z

magefiles/test.go

 	ds := Testds{}
 	c := Testcons{}
 	cover := parseCommandLineFlags()
-	ctxMyKey := ctxkey.New[bool]()
-	ctx := ctxMyKey.WithValue(context.Background(), cover)
+	ctx = ctxMyKey.WithValue(context.Background(), cover)


use the ctx argument provided by the method

yeah overlooked that, thanks for pointing out!

vroldanbet · 2025-02-28T09:38:37Z

magefiles/util.go

+	cover, err := ctxMyKey.Value(ctx)
+	if cover && err {


This is not idiomatic, because the second argument is not of error type, and that confuses the reader thinking it is an error. Also the if expression reads weird because it says if coverage is enabled and an error happened....

Methods that return value and a ok boolean are typically used like this:

if cover, ok := ctxMyKey.Value(ctx); cover && ok { }

Signed-off-by: Kartikay <[email protected]>

added test coverage

80503bb

Signed-off-by: Kartikay <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

kartikaysaxena added 2 commits February 14, 2025 15:58

Merge branch 'main' into mage-test-coverage

e7466ff

Merge branch 'main' into mage-test-coverage

c28c7b5

vroldanbet reviewed Feb 18, 2025

View reviewed changes

refactor ctx

dcdc4e4

Signed-off-by: Kartikay <[email protected]>

kartikaysaxena force-pushed the mage-test-coverage branch from 8c68cf0 to dcdc4e4 Compare February 20, 2025 19:37

kartikaysaxena requested a review from vroldanbet February 21, 2025 09:45

kartikaysaxena added 3 commits February 26, 2025 12:49

added test coverage

ce848be

Signed-off-by: Kartikay <[email protected]>

different profiles for tests

0853653

Signed-off-by: Kartikay <[email protected]>

use path hash instead

0ee517f

Signed-off-by: Kartikay <[email protected]>

kartikaysaxena and others added 3 commits February 26, 2025 12:49

separate targets for cover

2b7693b

Signed-off-by: Kartikay <[email protected]>

context coverage bool

bb75ace

Signed-off-by: Kartikay <[email protected]>

refactor ctx

8d38fc5

Signed-off-by: Kartikay <[email protected]>

tstirrat15 force-pushed the mage-test-coverage branch from dcdc4e4 to 8d38fc5 Compare February 26, 2025 19:49

tstirrat15 reviewed Feb 26, 2025

View reviewed changes

vroldanbet reviewed Feb 27, 2025

View reviewed changes

Merge branch 'mage-test-coverage' of https://github.com/kartikaysaxen…

67d5092

…a/spicedb into mage-test-coverage

github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Feb 27, 2025

vroldanbet requested changes Feb 27, 2025

View reviewed changes

magefiles/util.go Outdated Show resolved Hide resolved

kartikaysaxena force-pushed the mage-test-coverage branch from fce741b to b995558 Compare February 27, 2025 20:33

kartikaysaxena requested a review from vroldanbet February 27, 2025 20:45

vroldanbet reviewed Feb 28, 2025

View reviewed changes

global ctx

e00732f

Signed-off-by: Kartikay <[email protected]>

kartikaysaxena force-pushed the mage-test-coverage branch from b995558 to e00732f Compare February 28, 2025 12:58

kartikaysaxena requested a review from vroldanbet February 28, 2025 12:59

vroldanbet approved these changes Feb 28, 2025

View reviewed changes

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

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

vroldanbet Feb 18, 2025

kartikaysaxena Feb 20, 2025

vroldanbet Feb 18, 2025

vroldanbet Feb 18, 2025

vroldanbet Feb 18, 2025

vroldanbet Feb 18, 2025

kartikaysaxena Feb 20, 2025

tstirrat15 left a comment

tstirrat15 Feb 26, 2025

kartikaysaxena Feb 27, 2025

vroldanbet Feb 27, 2025 •

edited

Loading

kartikaysaxena Feb 27, 2025

vroldanbet Feb 27, 2025

vroldanbet Feb 28, 2025

kartikaysaxena Feb 28, 2025

vroldanbet Feb 28, 2025

		ctxMyKey := ctxkey.New[bool]()
		ctx := ctxMyKey.WithValue(context.Background(), cover)

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

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

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

tstirrat15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vroldanbet Feb 27, 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

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

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

vroldanbet Feb 27, 2025 •

edited

Loading