Skip to content

fix(utils): resolve incorrect line number reporting in FileWithLineNum#7716

Closed
huningzizhitaibai wants to merge 1 commit intogo-gorm:masterfrom
huningzizhitaibai:master
Closed

fix(utils): resolve incorrect line number reporting in FileWithLineNum#7716
huningzizhitaibai wants to merge 1 commit intogo-gorm:masterfrom
huningzizhitaibai:master

Conversation

@huningzizhitaibai
Copy link

The introduction of CallerFrame in v1.31.1 increased the call stack depth, but the hardcoded skip value in runtime.Callers was not adjusted. This caused FileWithLineNum to report its own caller's line number instead of the original business code's line number.

Also added a regression test to verify the fix.

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

Fixed a regression in FileWithLineNum introduced in v1.31.1. The new CallerFrame wrapper increased call stack depth, but runtime.Callers skip value was not updated. The skip value has been adjusted to correctly pinpoint the original caller.

User Case Description

When using v1.31.1 in the local project, FileWithLineNum() reports the line number of the internal FileWithLineNum() call itself instead of the business code line that triggered the GORM operation.

The introduction of CallerFrame in v1.31.1 increased the call stack
depth, but the hardcoded skip value in runtime.Callers was not adjusted.
This caused FileWithLineNum to report its own caller's line number
instead of the original business code's line number.

Also added a regression test to verify the fix.
@propel-code-bot
Copy link
Contributor

It also verifies both direct and closure call scenarios in the regression test to ensure correct line reporting across invocation patterns.

Affected Areas

utils/utils.go
utils/utils_test.go

This summary was automatically generated by @propel-code-bot

Comment on lines +219 to +221
if !strings.Contains(actual, expectedFile+":211") {
t.Errorf("Expected file path to contain %s, but got %s", expectedFile+":211", actual)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended

[Testing] Brittle test: This test hardcodes line number :211 which will break whenever code is added/removed above this line. The test comment acknowledges this fragility ("Update line numbers to match your environment"), making it unmaintainable.

A more robust approach would calculate the expected line dynamically:

func TestFileWithLineNum(t *testing.T) {
	_, _, expectedLine, _ := runtime.Caller(0)
	actual := where() // Called from next line
	expectedLine++ // where() is called one line after Caller(0)
	
	expectedFile := "utils_test.go"
	expectedStr := fmt.Sprintf("%s:%d", expectedFile, expectedLine)
	if !strings.Contains(actual, expectedStr) {
		t.Errorf("Expected %s, but got %s", expectedStr, actual)
	}
}

Or verify just the filename and that a line number exists:

if !strings.Contains(actual, expectedFile+":") || !strings.Contains(actual, "211") {
	// Verify format: filename:number
	parts := strings.Split(actual, ":")
	if len(parts) != 2 || !strings.HasSuffix(parts[0], expectedFile) {
		t.Errorf("Expected format 'utils_test.go:line_number', got %s", actual)
	}
}
Context for Agents
Brittle test: This test hardcodes line number `:211` which will break whenever code is added/removed above this line. The test comment acknowledges this fragility ("Update line numbers to match your environment"), making it unmaintainable.

A more robust approach would calculate the expected line dynamically:

```go
func TestFileWithLineNum(t *testing.T) {
	_, _, expectedLine, _ := runtime.Caller(0)
	actual := where() // Called from next line
	expectedLine++ // where() is called one line after Caller(0)
	
	expectedFile := "utils_test.go"
	expectedStr := fmt.Sprintf("%s:%d", expectedFile, expectedLine)
	if !strings.Contains(actual, expectedStr) {
		t.Errorf("Expected %s, but got %s", expectedStr, actual)
	}
}
```

Or verify just the filename and that a line number exists:

```go
if !strings.Contains(actual, expectedFile+":") || !strings.Contains(actual, "211") {
	// Verify format: filename:number
	parts := strings.Split(actual, ":")
	if len(parts) != 2 || !strings.HasSuffix(parts[0], expectedFile) {
		t.Errorf("Expected format 'utils_test.go:line_number', got %s", actual)
	}
}
```

File: utils/utils_test.go
Line: 221

Comment on lines +210 to +212
// 1. Test direct call scenario
actual := where()
fmt.Println(actual)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended

[Maintainability] Debug output left in test: fmt.Println(actual) and fmt.Println(closureResult) (lines 212, 229) should be removed. These print statements pollute test output and provide no value in automated test runs. They appear to be debug artifacts from development.

Suggested change
// 1. Test direct call scenario
actual := where()
fmt.Println(actual)
// 1. Test direct call scenario
actual := where()
// Expected result: Should output the correct line number of the caller.
Context for Agents
Debug output left in test: `fmt.Println(actual)` and `fmt.Println(closureResult)` (lines 212, 229) should be removed. These print statements pollute test output and provide no value in automated test runs. They appear to be debug artifacts from development.

```suggestion
	// 1. Test direct call scenario
	actual := where()

	// Expected result: Should output the correct line number of the caller.
```

File: utils/utils_test.go
Line: 212

@huningzizhitaibai
Copy link
Author

Sorry, I push my commit on master branch. I will change it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant