`redundant-test-main-exit` assumes that all `os.Exit` calls are based on the return from `m.Run`

[G-Rath]

Describe the bug
It seems that any use of os.Exit in TestMain is flagged by redundant-test-main-exit, regardless of if the return from m.Run is actually use

To Reproduce
Steps to reproduce the behavior:

1. I updated revive go install github.com/mgechev/revive@latest
2. I run it with the following flags & configuration file:

# flags

revive .

# config file
[rule.redundant-test-main-exit]

func TestMain(m *testing.M) {
	err := vcr.CleanCassettes()
	if err != nil {
		fmt.Println("Error cleaning cassettes:", err)
		os.Exit(1)
	}

	m.Run()

	dirty, err := snaps.Clean(m, snaps.CleanOpts{Sort: true})

	if err != nil {
		fmt.Println("Error cleaning snaps:", err)
		os.Exit(1)
	}
	if dirty {
		fmt.Println("Some snapshots were outdated.")
		os.Exit(1)
	}
}

A self-contained example:

// main_test.go
package main

import (
	"fmt"
	"os"
	"testing"
)

func cleanSnapshots() bool {
	// (in real life this actually does stuff)
	return false
}

func TestMain(m *testing.M) {
	m.Run()

	dirty := cleanSnapshots()

	if dirty {
		fmt.Println("Some snapshots were outdated.")
		os.Exit(1)
	}
}

Expected behavior
I expect nothing to be flagged, since none of my os.Exit calls are based on the return from m.Run

Logs
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: Ubuntu 22.04 via WSLv2 Win11
• Version of Go

Additional context
Add any other context about the problem here.

[ccoVeille]

I'm unsure I get the point.

For me there is no reason to use os.Exit in TestMain since Go 1.15

When they fixed this bad pattern that was breaking defer and cleanup.

A TestMain function is no longer required to call os.Exit. If a TestMain function returns, the test binary will call os.Exit with the value returned by m.Run.
https://go.dev/doc/go1.15#testingpkgtesting

So, for me, it's normal to report the os.Exit call in TestMain as they should not be present.

But maybe revive could report them in a better way

[G-Rath]

You still need to call os.Exit manually in TestMain if you want a non-zero exit code, such as because pre-test setup or post-test cleanup failed like in my example.

The only other option would be to panic which would mean any top-level defers get executed but has a more explosive output that could be misleading, and so I don't think it's fitting for this particular rule to enforce that (i.e if you were thinking of having the rule changed to be "use panic instead if you want to fail the suite"), though that could be useful to some as its own rule or an option

[G-Rath]

Also just noting that cleanup runs after the (sub)tests have completed, not when TestMain returns i.e. by the time m.Run is finished, cleanups have been run - so the use of os.Exit in TestMain only impacts defers within that same TestMain

[ccoVeille]

You are right.

That's the only way to invalidate a test when doing a teardowm that could fail

I found these issues

golang/go#57326

For me here, we are in a limited edge case scenario.

In your specific cases, what is reported is wrong. So it's a false-positive for you.

It could be fixed maybe, I'm unsure. It could lead to false-negative then.

Lets see what @alexandear or @chavacava think about it.

If I was you I would add a directive to ignore that line.

[G-Rath]

I think you should be able to reasonably improve the rule based on:

• is defer used in TestMain?
• is the return of m.Run assigned to a variable?

If the answer to both of those is "no", then you shouldn't need to report any os.Exit calls within TestMain, which would address this issue.

I think there's also other improvements to this rule based on different answers to those questions, such as if the answer is "yes" to the first question (regardless of the second), then any os.Exit calls should be reported as being an issue - but I don't think those all need to be necessarily accounted for in the one PR.

I would also suggest renaming the rule and updating the documentation as to me the use of "redundant" implies it only cares about the pattern of code := m.Run(); os.Exit(code) but what we're talking about with defer is outside of that - so it could be more appreciate to call it something like suspect-test-main-exit or risky-test-main-exit

[alexandear]

Thanks for raising this. It's a rare case, but definitely a false positive we need to fix.