-
-
Notifications
You must be signed in to change notification settings - Fork 732
coverageredesign: Map line directive file names to exec relative paths #4424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ package main | |
|
|
||
| import ( | ||
| "bytes" | ||
| "bufio" | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
|
|
@@ -194,3 +195,120 @@ func init() { | |
| } | ||
| return nil | ||
| } | ||
|
|
||
| // coveragePath returns the location path of coverage counter data emitted by | ||
| // the go runtime. With the go coverage redesign, the location path looks to be | ||
| // importpath + the file base name. Line directives are honored but the location | ||
| // path is still importpath relative. | ||
| func coveragePath(src string, importPath string) (string, error) { | ||
| directiveName, err := findFirstLineDirectiveFilename(src) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| filename := src | ||
| if directiveName != "" { | ||
| fmt.Println("directiveName: ", directiveName) | ||
| filename = directiveName | ||
| } | ||
| return importPath + "/" + filepath.Base(filename), nil | ||
| } | ||
|
|
||
| // lineDirective represents a parsed line directive | ||
| type lineDirective struct { | ||
| Filename string | ||
| Line int | ||
| Column int // 0 if not specified | ||
| } | ||
|
|
||
| // parseLineDirective extracts information from a Go line directive. | ||
| // It handles both //line and /*line*/ formats and validates the results. | ||
| func parseLineDirective(directive string) (*lineDirective, error) { | ||
| // Remove leading/trailing whitespace | ||
| directive = strings.TrimSpace(directive) | ||
|
|
||
| var content string | ||
|
|
||
| // Handle //line directive | ||
| if strings.HasPrefix(directive, "//line ") { | ||
| content = directive[7:] // Remove "//line " | ||
| } else if strings.HasPrefix(directive, "/*line ") && strings.HasSuffix(directive, "*/") { | ||
| // Handle /*line*/ directive | ||
| content = directive[7 : len(directive)-2] // Remove "/*line " and "*/" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace constants with |
||
| } else { | ||
| return nil, fmt.Errorf("not a valid line directive") | ||
| } | ||
|
|
||
| if content == "" { | ||
| return nil, fmt.Errorf("empty line directive content") | ||
| } | ||
|
|
||
| return parseLineContent(content) | ||
| } | ||
|
|
||
| // parseLineContent parses the content after "line " prefix | ||
| func parseLineContent(content string) (*lineDirective, error) { | ||
| // Use a more sophisticated approach to handle Windows paths | ||
| // Find all colons and work backwards to find line/column numbers | ||
| parts := strings.Split(content, ":") | ||
| if len(parts) < 2 { | ||
| return nil, fmt.Errorf("missing line number") | ||
| } | ||
|
|
||
| // Try to parse as filename:line:col | ||
| if len(parts) >= 3 { | ||
| // Check if last two parts are valid numbers | ||
| colStr := parts[len(parts)-1] | ||
| lineStr := parts[len(parts)-2] | ||
|
|
||
| col, colErr := strconv.Atoi(colStr) | ||
| line, lineErr := strconv.Atoi(lineStr) | ||
|
|
||
| if colErr == nil && lineErr == nil && line > 0 && col > 0 { | ||
| // Valid filename:line:col format | ||
| filename := strings.Join(parts[:len(parts)-2], ":") | ||
| return &lineDirective{ | ||
| Filename: filename, | ||
| Line: line, | ||
| Column: col, | ||
| }, nil | ||
| } | ||
| } | ||
|
|
||
| // Try to parse as filename:line | ||
| lineStr := parts[len(parts)-1] | ||
| line, err := strconv.Atoi(lineStr) | ||
| if err != nil || line <= 0 { | ||
| return nil, fmt.Errorf("invalid line number: %s", lineStr) | ||
| } | ||
|
|
||
| filename := strings.Join(parts[:len(parts)-1], ":") | ||
|
|
||
| return &lineDirective{ | ||
| Filename: filename, | ||
| Line: line, | ||
| Column: 0, | ||
| }, nil | ||
| } | ||
|
|
||
| // findFirstLineDirectiveFilename opens a Go file, scans it, and returns the | ||
| // filename from the first line directive it finds. | ||
| func findFirstLineDirectiveFilename(filePath string) (string, error) { | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("open file: %w", err) | ||
| } | ||
| defer file.Close() | ||
|
|
||
| scanner := bufio.NewScanner(file) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if ld, err := parseLineDirective(line); err == nil && ld != nil { | ||
| return ld.Filename, nil // Found the first one, return it. | ||
| } | ||
| } | ||
| if err := scanner.Err(); err != nil { | ||
| return "", fmt.Errorf("scanning file: %w", err) | ||
| } | ||
| // no line directive found, return empty string | ||
| return "", nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,3 +113,44 @@ func TestRegisterCoverage(t *testing.T) { | |
| } | ||
| } | ||
| } | ||
|
|
||
| func TestFindFirstLineDirectiveFilename(t *testing.T) { | ||
| tests := []struct { | ||
| desc string | ||
| src string | ||
| content string | ||
| want string | ||
| }{ | ||
| {desc: "no line directive", src: "noline.go", content: "package main\n\nfunc main() {}\n", want: ""}, | ||
| {desc: "slash slash directive", src: "slashslash.go", content: "package main\n\n//line foo.go:123\n\nfunc main() {}\n", want: "foo.go"}, | ||
| {desc: "slash star directive", src: "slashstar.go", content: "package main\n\n/*line foo.go:123*/", want: "foo.go"}, | ||
| {desc: "windows absolute path", src: "windows.go", content: "package main\n\n//line C:\\Windows\\path.go:300", want: "C:\\Windows\\path.go"}, | ||
| {desc: "windows with column", src: "windows.go", content: "package main\n\n//line C:\\Windows\\path.go:300:10", want: "C:\\Windows\\path.go"}, | ||
| {desc: "url-like path", src: "url.go", content: "package main\n\n//line http://example.com/file.go:400", want: "http://example.com/file.go"}, | ||
| {desc: "multiple colons", src: "multiple.go", content: "package main\n\n//line scheme:rest:file.go:500", want: "scheme:rest:file.go"}, | ||
| {desc: "empty filename", src: "empty.go", content: "package main\n\n//line :50", want: ""}, | ||
| {desc: "invalid line number", src: "invalid.go", content: "package main\n\n//line file.go:0", want: ""}, | ||
| {desc: "invalid line number", src: "invalid.go", content: "package main\n\n//line file.go:invalid", want: ""}, | ||
| {desc: "not a directive", src: "notadirective.go", content: "package main\n\nnot a line directive", want: ""}, | ||
| {desc: "incomplete", src: "incomplete.go", content: "package main\n\n//line", want: ""}, | ||
| {desc: "missing line number", src: "missing.go", content: "package main\n\n//line file.go:", want: ""}, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| testTempDir := t.TempDir() | ||
| testFile := filepath.Join(testTempDir, test.src) | ||
| err := os.WriteFile(testFile, []byte(test.content), 0644) | ||
| if err != nil { | ||
| t.Errorf("create test file: %v", err) | ||
| } | ||
| defer os.Remove(testFile) // Clean up the test file. | ||
|
|
||
| got, err := findFirstLineDirectiveFilename(testFile) | ||
| if err != nil { | ||
| t.Errorf("find first line directive filename: %v", err) | ||
| } | ||
| if got != test.want { | ||
| t.Errorf("find first line directive filename: got %q, want %q", got, test.want) | ||
| } | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I don't think testing an unexported function like this is useful. It forces future maintainers/contributors to study the implementation details when doing refactoring. I would prefer it if we could add a high-level test, say in |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug print?