Handle failure of runtime.Caller

[lazysegtree]

Issue - #2

[lazysegtree]

Testcase run

➜  ~/Workspace/kuknitin/assert-go git:(main) [9:42:05] git log --oneline | head -n 2
152c645 Handle failure of runtime.Caller
1e10074 docs(readme): add personal perspective about assertions in Go
➜  ~/Workspace/kuknitin/assert-go git:(main) [9:42:15] go test
PASS
ok  	github.com/nikoksr/assert-go	0.263s
➜  ~/Workspace/kuknitin/assert-go git:(main) [9:42:22]

[lazysegtree]

Testing the failure scenario code flow by locally updating the skip value to 10. Thats the only way I have for now to make runtime.Caller() fail deterministically

➜  ~/Workspace/kuknitin/assert-go git:(main) ✗ [9:54:17] cat assert.go | grep -n runtime.Caller
42:	_, file, line, ok := runtime.Caller(10) //nolint:mnd // Explained in comment
➜  ~/Workspace/kuknitin/assert-go git:(main) ✗ [9:54:22]

# Using locally changed module
(.venv) ➜  ~/Workspace/kuknitin/shared/Misc/Go/Learning/basics/assert git:(main) ✗ [9:50:50] go mod edit -replace "github.com/nikoksr/assert-go"=/Users/kuknitin/Workspace/kuknitin/assert-go

(.venv) ➜  ~/Workspace/kuknitin/shared/Misc/Go/Learning/basics/assert git:(main) ✗ [10:02:22] go run main.go     
panic: Assertion failed (Runtime caller info is not available)
        Message: Failing assertion


goroutine 1 [running]:
github.com/nikoksr/assert-go.assert(0x68?, {0x104ac80be, 0x11}, {0x0, 0x0, 0x0})
        /Users/kuknitin/Workspace/kuknitin/assert-go/assert.go:46 +0x32c
github.com/nikoksr/assert-go.Assert(...)
        /Users/kuknitin/Workspace/kuknitin/assert-go/assert.go:30
main.main()
        /Users/kuknitin/Workspace/kuknitin/shared/Misc/Go/Learning/basics/assert/main.go:6 +0x3c
exit status 2
(.venv) ➜  ~/Workspace/kuknitin/shared/Misc/Go/Learning/basics/assert git:(main) ✗ [10:02:24] 

main.go

package main 
import (
	"github.com/nikoksr/assert-go"
)
func main() {
	assert.Assert(false, "Failing assertion")
}

[nikoksr]

Hi @lazysegtree! First of all, I appreciate your contribution and I agree with your changes.

I got curious about the problem to deterministically test this behavior. Locally, I came up with the following changes which, unless I don't fully understand the problem you're describing, allow me to deterministically test this.

---

First, I added a private, global variable which allows us to change the amount of frames to be skipped. This will be the value that's set to 2 by default but can be altered by our test case to something much larger, e.g. 1000:

assert.go:

// skipFrames is the number of stack frames to skip when getting the source context. By default, we skip 2 frames:
//  1. The assert() function itself
//  2. The Assert()/Debug() function that called assert()
//
// The purpose of this variable is to enable deterministic testing of the source context as described here:
//   - https://github.com/nikoksr/assert-go/issues/2
//
//nolint:gochecknoglobals // required for testing, otherwise read-only
var skipFrames = 2

I then altered the assert function, so that it utilizes this variable:

assert.go:

// assert panics if the condition is false. Configurable via SetConfig.
func assert(condition bool, msg string, values ...any) {
	if condition {
		return // Assertion met
	}

	// Skip stack frames
	_, file, line, ok := runtime.Caller(skipFrames)

	// Could not get Caller info
	if !ok {
		panic(AssertionError{
			Message: msg,
		})
	}

	// ...

}

And finally added a test case to assert_test.go:

// ...
	
func TestAssertCallerFailure(t *testing.T) {
	// Capture the original skip value
	oldSkipFrames := skipFrames
	// Set skip to a value that will definitely exceed call stack depth
	skipFrames = 1000
	// Restore the original value after test
	defer func() { skipFrames = oldSkipFrames }()

	// Capture and verify the panic
	defer func() {
		r := recover()
		if r == nil {
			t.Fatal("Expected panic, but none occurred")
		}

		ae, ok := r.(AssertionError)
		if !ok {
			t.Fatalf("Expected AssertionError, got %T", r)
		}

		// Verify we got the simplified error without file/line info
		if ae.File != "" {
			t.Errorf("Expected empty file, got %q", ae.File)
		}
		if ae.Line != 0 {
			t.Errorf("Expected line to be 0, got %d", ae.Line)
		}
		if ae.SourceContext != "" {
			t.Errorf("Expected empty source context, got %q", ae.SourceContext)
		}
		// Message should still be included
		if ae.Message == "" {
			t.Error("Expected non-empty message")
		}
	}()

	Assert(false, "This should fail to get caller info")
}

---

What are your thoughts on this? If you agree with my suggestions, would you like to apply them? Otherwise I can push my changes. Cheers.

[lazysegtree]

Thanks for your suggestion. I feel that having a global variable and then modifying it in test cases, and defering the rollback of modification is a bit too hacky.
Now, we want to function assert to behave differently based of skipFrames variable. So rather than having it read from a global variable, we should pass it as a parameter. That makes the data flow in the program more clear. And is more idiomatic.

Check the latest commit if it looks good.

@nikoksr

[lazysegtree]

I agree that there is a minor increased overhead due to passing one more parameter. But this is a trade-off between micro-optimization vs cleaner and more maintainable codebase.

[lazysegtree]

test run

➜  ~/Workspace/kuknitin/assert-go git:(main) [7:27:38] git log --oneline | head -n 4
a579f52 fix: Use a more strict check in test, add skipFrame value in Debug()
13d733a test: Add test for failure of caller info, and pass skipFrames as parameter
5bc238f feat: Handle failure of runtime.Caller
1e10074 docs(readme): add personal perspective about assertions in Go
➜  ~/Workspace/kuknitin/assert-go git:(main) [7:27:40] go test ./...
ok  	github.com/nikoksr/assert-go	(cached)
➜  ~/Workspace/kuknitin/assert-go git:(main) [7:27:45] go test -tags assertdebug ./...
ok  	github.com/nikoksr/assert-go	(cached)
➜  ~/Workspace/kuknitin/assert-go git:(main) [7:27:47]

[nikoksr]

Thanks for your suggestion. I feel that having a global variable and then modifying it in test cases, and defering the rollback of modification is a bit too hacky. Now, we want to function assert to behave differently based of skipFrames variable. So rather than having it read from a global variable, we should pass it as a parameter. That makes the data flow in the program more clear. And is more idiomatic.

Check the latest commit if it looks good.

@nikoksr

I appreciate that feedback. I'm fine merging your implementation after the review.

I agree that there is a minor increased overhead due to passing one more parameter. But this is a trade-off between micro-optimization vs cleaner and more maintainable codebase.

I don't agree with this. I'm rather certain there'd be virtually no overhead due to compiler optimizations. It will most probably be inlined by the compiler. I have not verified this though as I'm fine with it either way. The actual overhead of runtime.Caller would completely dwarf any theoretical differences in how the skip frames value is passed anyway.

[lazysegtree]

@nikoksr Applied the changes about moving comments.

[lazysegtree]

@nikoksr Sorry. I didn't see that. I have fixed it now.

[nikoksr]

@lazysegtree thanks again for your thorough contribution ✌🏻

[nikoksr]

Closed #2