Skip to content

Commit a62fd5a

Browse files
kalverrakalverra
committed
Cleaner errors
1 parent 5389d9a commit a62fd5a

File tree

3 files changed

+71
-129
lines changed

3 files changed

+71
-129
lines changed

‎tools/flakeguard/cmd/make_pr.go‎

Lines changed: 32 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -20,29 +20,15 @@ import (
2020
"github.com/smartcontractkit/chainlink-testing-framework/tools/flakeguard/localdb"
2121
)
2222

23-
const (
24-
openAIKeyEnvVar = "OPENAI_API_KEY"
25-
openAIKeyFlag = "openai-key"
26-
)
27-
2823
var (
2924
repoPath string
3025
localDBPath string
31-
openAIKey string
3226
)
3327

3428
var MakePRCmd = &cobra.Command{
3529
Use: "make-pr",
3630
Short: "Make a PR to skip identified flaky tests",
37-
PersistentPreRun: func(cmd *cobra.Command, args []string) {
38-
if !cmd.Flag(openAIKeyFlag).Changed {
39-
openAIKey = os.Getenv(openAIKeyEnvVar)
40-
}
41-
if openAIKey == "" {
42-
fmt.Printf("Warning: OpenAI API key is not provided by flag '%s' or env var '%s', cannot use LLM to skip or fix flaky tests\n", openAIKeyFlag, openAIKeyEnvVar)
43-
}
44-
},
45-
RunE: makePR,
31+
RunE: makePR,
4632
}
4733

4834
func makePR(cmd *cobra.Command, args []string) error {
@@ -103,29 +89,28 @@ func makePR(cmd *cobra.Command, args []string) error {
10389
return fmt.Errorf("failed to checkout new branch: %w", err)
10490
}
10591

106-
// DEBUG: Maybe this shouldn't be used ever? Debugging is harder without the branch around.
107-
// cleanUpBranch := true
108-
// defer func() {
109-
// if cleanUpBranch {
110-
// fmt.Printf("Cleaning up branch %s...", branchName)
111-
// // First checkout default branch
112-
// err = targetRepoWorktree.Checkout(&git.CheckoutOptions{
113-
// Branch: plumbing.NewBranchReferenceName(defaultBranch),
114-
// Force: true, // Force checkout to discard any changes for a clean default branch
115-
// })
116-
// if err != nil {
117-
// fmt.Printf("Failed to checkout default branch: %v\n", err)
118-
// return
119-
// }
120-
// // Then delete the local branch
121-
// err = repo.Storer.RemoveReference(plumbing.NewBranchReferenceName(branchName))
122-
// if err != nil {
123-
// fmt.Printf("Failed to remove local branch: %v\n", err)
124-
// return
125-
// }
126-
// fmt.Println(" ✅")
127-
// }
128-
// }()
92+
cleanUpBranch := true
93+
defer func() {
94+
if cleanUpBranch {
95+
fmt.Printf("Cleaning up branch %s...", branchName)
96+
// First checkout default branch
97+
err = targetRepoWorktree.Checkout(&git.CheckoutOptions{
98+
Branch: plumbing.NewBranchReferenceName(defaultBranch),
99+
Force: true, // Force checkout to discard any changes for a clean default branch
100+
})
101+
if err != nil {
102+
fmt.Printf("Failed to checkout default branch: %v\n", err)
103+
return
104+
}
105+
// Then delete the local branch
106+
err = repo.Storer.RemoveReference(plumbing.NewBranchReferenceName(branchName))
107+
if err != nil {
108+
fmt.Printf("Failed to remove local branch: %v\n", err)
109+
return
110+
}
111+
fmt.Println(" ✅")
112+
}
113+
}()
129114

130115
if len(currentlyFlakyEntries) == 0 {
131116
fmt.Println("No flaky tests found!")
@@ -143,7 +128,7 @@ func makePR(cmd *cobra.Command, args []string) error {
143128
jiraTickets = append(jiraTickets, entry.JiraTicket)
144129
}
145130

146-
err = golang.SkipTests(repoPath, openAIKey, testsToSkip)
131+
err = golang.SkipTests(repoPath, testsToSkip)
147132
if err != nil {
148133
return fmt.Errorf("failed to modify code to skip tests: %w", err)
149134
}
@@ -178,7 +163,6 @@ func makePR(cmd *cobra.Command, args []string) error {
178163
skippedTestsPRBody strings.Builder
179164
alreadySkippedTestsPRBody strings.Builder
180165
errorSkippingTestsPRBody strings.Builder
181-
llmSkippedTestsPRBody strings.Builder
182166
)
183167

184168
for _, test := range testsToSkip {
@@ -196,11 +180,6 @@ func makePR(cmd *cobra.Command, args []string) error {
196180
alreadySkippedTestsPRBody.WriteString(fmt.Sprintf("- Package: `%s`\n", test.Package))
197181
alreadySkippedTestsPRBody.WriteString(fmt.Sprintf(" Test: `%s`\n", test.Name))
198182
alreadySkippedTestsPRBody.WriteString(fmt.Sprintf(" Ticket: [%s](https://%s/browse/%s)\n", test.JiraTicket, os.Getenv("JIRA_DOMAIN"), test.JiraTicket))
199-
} else if test.LLMSkipped {
200-
llmSkippedTestsPRBody.WriteString(fmt.Sprintf("- Package: `%s`\n", test.Package))
201-
llmSkippedTestsPRBody.WriteString(fmt.Sprintf(" Test: `%s`\n", test.Name))
202-
llmSkippedTestsPRBody.WriteString(fmt.Sprintf(" Ticket: [%s](https://%s/browse/%s)\n", test.JiraTicket, os.Getenv("JIRA_DOMAIN"), test.JiraTicket))
203-
llmSkippedTestsPRBody.WriteString(fmt.Sprintf(" [View skip in PR](https://github.com/%s/%s/pull/%s/files#diff-%sL%d)\n\n", owner, repoName, branchName, commitHash, test.Line))
204183
}
205184
}
206185
body := fmt.Sprintf(`## Tests That I Failed to Skip, Need Manual Intervention
@@ -211,13 +190,9 @@ func makePR(cmd *cobra.Command, args []string) error {
211190
212191
%s
213192
214-
## Tests Skipped Using LLM Assistance
215-
216-
%s
217-
218193
## Tests That Were Already Skipped
219194
220-
%s`, errorSkippingTestsPRBody.String(), skippedTestsPRBody.String(), llmSkippedTestsPRBody.String(), alreadySkippedTestsPRBody.String())
195+
%s`, errorSkippingTestsPRBody.String(), skippedTestsPRBody.String(), alreadySkippedTestsPRBody.String())
221196

222197
pr := &github.NewPullRequest{
223198
Title: github.Ptr(fmt.Sprintf("[%s] Flakeguard: Skip flaky tests", strings.Join(jiraTickets, "] ["))),
@@ -227,13 +202,15 @@ func makePR(cmd *cobra.Command, args []string) error {
227202
MaintainerCanModify: github.Ptr(true),
228203
}
229204

230-
fmt.Println("PR Preview:")
231-
fmt.Println("================================================")
232-
fmt.Println(pr.Title)
205+
fmt.Println()
206+
fmt.Println("*** PR Preview ***")
207+
fmt.Println()
208+
fmt.Println(pr.GetTitle())
233209
fmt.Println("--------------------------------")
234210
fmt.Printf("Merging '%s' into '%s'\n", branchName, defaultBranch)
235-
fmt.Println(pr.Body)
211+
fmt.Println(pr.GetBody())
236212
fmt.Println("================================================")
213+
fmt.Println()
237214

238215
fmt.Printf("To preview the code changes in the GitHub UI, visit: https://github.com/%s/%s/compare/%s...%s\n", owner, repoName, defaultBranch, branchName)
239216
fmt.Print("Would you like to create the PR automatically from the CLI? (y/N): ")
@@ -261,11 +238,11 @@ func makePR(cmd *cobra.Command, args []string) error {
261238
return fmt.Errorf("failed to create PR, got bad status: %s\n%s", resp.Status, string(body))
262239
}
263240

241+
cleanUpBranch = false
264242
fmt.Printf("PR created! https://github.com/%s/%s/pull/%d\n", owner, repoName, createdPR.GetNumber())
265243
return nil
266244
}
267245

268246
func init() {
269247
MakePRCmd.Flags().StringVarP(&repoPath, "repoPath", "r", ".", "Local path to the repository to make the PR for")
270-
MakePRCmd.Flags().StringVarP(&openAIKey, openAIKeyFlag, "k", "", fmt.Sprintf("OpenAI API key for using an LLM to help create the PR (can be set via %s environment variable)", openAIKeyEnvVar))
271248
}

‎tools/flakeguard/golang/golang.go‎

Lines changed: 38 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package golang
22

33
import (
4-
"bufio"
54
"bytes"
65
"encoding/json"
76
"fmt"
@@ -23,13 +22,13 @@ type Package struct {
2322
Dir string `json:"Dir"`
2423
ImportPath string `json:"ImportPath"`
2524
Root string `json:"Root"`
26-
Deps []string `json:"Deps"`
27-
TestImports []string `json:"TestImports"`
28-
XTestImports []string `json:"XTestImports"`
29-
GoFiles []string `json:"GoFiles"`
30-
TestGoFiles []string `json:"TestGoFiles"`
31-
XTestGoFiles []string `json:"XTestGoFiles"`
32-
EmbedFiles []string `json:"EmbedFiles"`
25+
Deps []string `json:"Deps,omitempty"`
26+
TestImports []string `json:"TestImports,omitempty"`
27+
XTestImports []string `json:"XTestImports,omitempty"`
28+
GoFiles []string `json:"GoFiles,omitempty"`
29+
TestGoFiles []string `json:"TestGoFiles,omitempty"`
30+
XTestGoFiles []string `json:"XTestGoFiles,omitempty"`
31+
EmbedFiles []string `json:"EmbedFiles,omitempty"`
3332
}
3433

3534
type DepMap map[string][]string
@@ -57,39 +56,25 @@ func Packages(repoPath string) ([]Package, error) {
5756
if err != nil {
5857
return fmt.Errorf("error getting packages: %w\nOutput:\n%s", err, string(out))
5958
}
60-
scanner := bufio.NewScanner(bytes.NewReader(out))
61-
var (
62-
buffer bytes.Buffer
63-
inJSON bool
64-
)
65-
66-
for scanner.Scan() {
67-
line := scanner.Text()
68-
line = strings.TrimSpace(line)
69-
buffer.WriteString(line)
70-
if !inJSON && !strings.HasPrefix(line, "{") {
71-
continue
72-
}
73-
if !inJSON && strings.HasPrefix(line, "{") {
74-
inJSON = true
75-
}
76-
77-
if line == "}" {
78-
inJSON = false
79-
var pkg Package
80-
if err := json.Unmarshal(buffer.Bytes(), &pkg); err != nil {
81-
_ = os.WriteFile(filepath.Join("go_list_output.json"), buffer.Bytes(), 0644)
82-
return fmt.Errorf("error unmarshalling go list output for dir '%s', see 'go_list_output.json' for output: %w", cmd.Dir, err)
83-
}
84-
packages = append(packages, pkg)
85-
buffer.Reset()
86-
}
59+
// output sometimes contains go downloading things, need to strip that out
60+
jsonStart := bytes.IndexByte(out, '{')
61+
if jsonStart == -1 {
62+
return fmt.Errorf("no JSON found in go list output for dir '%s': go list output:\n%s", cmd.Dir, string(out))
8763
}
88-
89-
if err := scanner.Err(); err != nil {
90-
return fmt.Errorf("error scanning go list output for file %s: %w", path, err)
64+
cleanOutput := out[jsonStart:]
65+
66+
// go list -json outputs multiple JSON objects (one per package), not a JSON array
67+
// So we need to decode each JSON object individually
68+
var pkgs []Package
69+
decoder := json.NewDecoder(bytes.NewReader(cleanOutput))
70+
for decoder.More() {
71+
var pkg Package
72+
if err := decoder.Decode(&pkg); err != nil {
73+
return fmt.Errorf("error unmarshalling go list output for dir '%s': %w\ngo list output:\n%s", cmd.Dir, err, string(cleanOutput))
74+
}
75+
pkgs = append(pkgs, pkg)
9176
}
92-
return nil
77+
packages = append(packages, pkgs...)
9378
}
9479
return nil
9580
})
@@ -248,15 +233,14 @@ type SkipTest struct {
248233
// Set by SkipTests
249234
// These values might be useless info, but for now we'll keep them
250235
SimplySkipped bool // If the test was newly skipped using simple AST parsing methods
251-
LLMSkipped bool // If the test was newly skipped using LLM assistance
252236
AlreadySkipped bool // If the test was already skipped before calling SkipTests
253237
ErrorSkipping error // If we failed to skip the test, this will be set
254238
File string // The file that the test was skipped in
255239
Line int
256240
}
257241

258242
// SkipTests finds all package/test pairs provided and skips them
259-
func SkipTests(repoPath, openAIKey string, testsToSkip []*SkipTest) error {
243+
func SkipTests(repoPath string, testsToSkip []*SkipTest) error {
260244
packages, err := Packages(repoPath)
261245
if err != nil {
262246
return err
@@ -276,34 +260,11 @@ func SkipTests(repoPath, openAIKey string, testsToSkip []*SkipTest) error {
276260
}
277261
}
278262
if packageDir == "" {
279-
// Print all packages to a file to help debug
280-
type PackageInfo struct {
281-
Dir string `json:"dir"`
282-
ImportPath string `json:"importPath"`
283-
Root string `json:"root"`
284-
GoFiles []string `json:"goFiles"`
285-
TestGoFiles []string `json:"testGoFiles"`
286-
}
287-
288-
packageInfos := make([]PackageInfo, len(packages))
289-
for i, pkg := range packages {
290-
packageInfos[i] = PackageInfo{
291-
Dir: pkg.Dir,
292-
ImportPath: pkg.ImportPath,
293-
Root: pkg.Root,
294-
GoFiles: pkg.GoFiles,
295-
TestGoFiles: pkg.TestGoFiles,
296-
}
297-
}
298-
299-
packagesJSON, err := json.MarshalIndent(packageInfos, "", " ")
300-
if err != nil {
301-
return fmt.Errorf("error marshalling packages: %w", err)
302-
}
303-
if err := os.WriteFile("packages_debug.json", packagesJSON, 0644); err != nil {
304-
return fmt.Errorf("error writing packages.json: %w", err)
305-
}
306-
return fmt.Errorf("directory for package '%s' not found, see 'packages_debug.json' for all found packages", packageImportPath)
263+
testToSkip.ErrorSkipping = fmt.Errorf(
264+
"unable to find a directory for package '%s' to skip '%s', package may have moved or been deleted",
265+
packageImportPath, testToSkip.Name,
266+
)
267+
return nil
307268
}
308269

309270
err := filepath.Walk(packageDir, func(path string, info os.FileInfo, err error) error {
@@ -345,9 +306,15 @@ func SkipTests(repoPath, openAIKey string, testsToSkip []*SkipTest) error {
345306
}
346307
if !found {
347308
if testToSkip.ErrorSkipping == nil {
348-
testToSkip.ErrorSkipping = fmt.Errorf("test '%s' not found in package '%s'", testToSkip.Name, testToSkip.Package)
309+
testToSkip.ErrorSkipping = fmt.Errorf(
310+
"cannot find '%s' in package '%s', the test may be tricky for me to find, or may have moved or been deleted",
311+
testToSkip.Name, testToSkip.Package,
312+
)
349313
} else {
350-
testToSkip.ErrorSkipping = fmt.Errorf("error skipping test '%s' in package '%s': %w", testToSkip.Name, testToSkip.Package, testToSkip.ErrorSkipping)
314+
testToSkip.ErrorSkipping = fmt.Errorf(
315+
"error skipping '%s' in package '%s': %w",
316+
testToSkip.Name, testToSkip.Package, testToSkip.ErrorSkipping,
317+
)
351318
}
352319
log.Warn().
353320
Str("test", testToSkip.Name).

‎tools/flakeguard/golang/golang_test.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,17 +119,15 @@ func TestSkipTests(t *testing.T) {
119119
slices.Sort(expectedSkipped)
120120
// End setup
121121

122-
err = SkipTests(testRunDir, "", testsToSkip)
122+
err = SkipTests(testRunDir, testsToSkip)
123123
assert.NoError(t, err)
124124

125125
for _, test := range testsToSkip {
126126
if test.Name == "TestPackASkippedAlready" || test.Name == "TestPackBSkippedAlready" {
127127
assert.True(t, test.AlreadySkipped, "Expected already skipped test '%s' to already be skipped", test.Name)
128128
assert.False(t, test.SimplySkipped, "Expected already skipped test '%s' to not be skipped again", test.Name)
129-
assert.False(t, test.LLMSkipped, "Expected already skipped test '%s' to not be skipped again", test.Name)
130129
} else {
131130
assert.True(t, test.SimplySkipped, "Expected flaky test '%s' to be simply skipped", test.Name)
132-
assert.False(t, test.LLMSkipped, "Expected flaky test '%s' to be simply skipped", test.Name)
133131
assert.False(t, test.AlreadySkipped, "Test '%s' should not have already been skipped", test.Name)
134132
}
135133
assert.NoError(t, test.ErrorSkipping, "Expected no error skipping test")

0 commit comments

Comments
 (0)