Skip to content

Commit 2a20b7c

Browse files
jskeetjskeet
authored
tests(internal): avoid a panic when expecting an error (#2217)
These changes all use Fatal or Fatalf to abort a test when an expected error isn't returned (i.e. `err == nil`). This avoids a panic when further checks are performed on the error, typically checking its message. Some tests used if/else-if to avoid this panic. This change updates all tests to a consistent style. For internal/sidekick, this only changes the "definitely wrong" cases. There are some other tests which are currently fine due to only checking whether or not there *is* an error, but which would need modification if the test were changed to check for a particular error message. Some other tests are Fixes #2216
1 parent c4c95a5 commit 2a20b7c

File tree

16 files changed

+83
-73
lines changed

16 files changed

+83
-73
lines changed

‎internal/automation/cli_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ func TestParseArgs(t *testing.T) {
142142
t.Run(test.name, func(t *testing.T) {
143143
got, err := parseFlags(test.args)
144144
if test.wantErr && err == nil {
145-
t.Errorf("expected error, but did not return one")
145+
t.Fatal("expected error, but did not return one")
146146
} else if !test.wantErr && err != nil {
147147
t.Errorf("did not expect error, but received one: %s", err)
148148
}

‎internal/automation/trigger_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ func TestRunCommandWithClient(t *testing.T) {
195195
}
196196
err := runCommandWithClient(ctx, client, ghClient, test.command, "some-project", test.push, test.build, test.forceRun, test.dateTime)
197197
if test.wantErr && err == nil {
198-
t.Errorf("expected error, but did not return one")
198+
t.Fatal("expected error, but did not return one")
199199
} else if !test.wantErr && err != nil {
200200
t.Errorf("did not expect error, but received one: %s", err)
201201
}
@@ -345,7 +345,7 @@ func TestRunCommandWithConfig(t *testing.T) {
345345
}
346346
err := runCommandWithConfig(ctx, client, ghClient, test.command, "some-project", true, true, test.forceRun, test.config, test.dateTime)
347347
if test.wantErr && err == nil {
348-
t.Errorf("expected error, but did not return one")
348+
t.Fatal("expected error, but did not return one")
349349
} else if !test.wantErr && err != nil {
350350
t.Errorf("did not expect error, but received one: %s", err)
351351
}

‎internal/config/config_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ func TestValidateHostMount(t *testing.T) {
545545
ok, err := validateHostMount(test.hostMount, test.defaultMount)
546546
if test.wantErr {
547547
if err == nil {
548-
t.Error("validateHostMount() should return error")
548+
t.Fatal("validateHostMount() should return error")
549549
}
550550

551551
if !strings.Contains(err.Error(), test.wantErrMsg) {

‎internal/config/librarian_config_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ func TestGlobalConfig_Validate(t *testing.T) {
8686
err := test.config.Validate()
8787
if test.wantErr {
8888
if err == nil {
89-
t.Errorf("GlobalConfig.Validate() should return error")
89+
t.Fatal("GlobalConfig.Validate() should return error")
9090
}
9191

9292
if !strings.Contains(err.Error(), test.wantErrMsg) {

‎internal/config/state_test.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,9 @@ func TestLibrarianState_Validate(t *testing.T) {
9494
err := test.state.Validate()
9595
if test.wantErr {
9696
if err == nil {
97-
t.Error("Librarian.Validate() should fail")
98-
} else if !strings.Contains(err.Error(), test.wantErrMsg) {
97+
t.Fatal("Librarian.Validate() should fail")
98+
}
99+
if !strings.Contains(err.Error(), test.wantErrMsg) {
99100
t.Errorf("want error message %q, got %q", test.wantErrMsg, err.Error())
100101
}
101102

@@ -316,7 +317,7 @@ func TestLibrary_Validate(t *testing.T) {
316317
err := test.library.Validate()
317318
if test.wantErr {
318319
if err == nil {
319-
t.Error("Library.Validate() should fail")
320+
t.Fatal("Library.Validate() should fail")
320321
}
321322
if !strings.Contains(err.Error(), test.wantErrMsg) {
322323
t.Errorf("want error message %q, got %q", test.wantErrMsg, err.Error())
@@ -362,7 +363,7 @@ func TestAPI_Validate(t *testing.T) {
362363
err := test.api.Validate()
363364
if test.wantErr {
364365
if err == nil {
365-
t.Error("API.Validate() should fail")
366+
t.Fatal("API.Validate() should fail")
366367
}
367368
if !strings.Contains(err.Error(), test.wantErrMsg) {
368369
t.Errorf("want error message %q, got %q", test.wantErrMsg, err.Error())

‎internal/conventionalcommits/conventional_commits_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,7 @@ END_COMMIT_OVERRIDE`,
463463
got, err := ParseCommits(commit, "example-id")
464464
if test.wantErr {
465465
if err == nil {
466-
t.Errorf("%s should return error", test.name)
466+
t.Fatalf("%s should return error", test.name)
467467
}
468468
if !strings.Contains(err.Error(), test.wantErrPhrase) {
469469
t.Errorf("ParseCommits(%q) returned error %q, want to contain %q", test.message, err.Error(), test.wantErrPhrase)

‎internal/docker/docker_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,7 @@ func TestDockerRun(t *testing.T) {
521521

522522
if test.wantErr {
523523
if err == nil {
524-
t.Errorf("%s should return error", test.name)
524+
t.Fatalf("%s should return error", test.name)
525525
}
526526
if !strings.Contains(err.Error(), test.wantErrMsg) {
527527
t.Errorf("want error message: %s, got: %s", test.wantErrMsg, err.Error())
@@ -620,7 +620,7 @@ func TestWriteLibraryState(t *testing.T) {
620620

621621
if test.wantErr {
622622
if err == nil {
623-
t.Errorf("writeLibraryState() shoud fail")
623+
t.Fatalf("writeLibraryState() shoud fail")
624624
}
625625

626626
if !strings.Contains(err.Error(), test.wantErrMsg) {
@@ -731,7 +731,7 @@ func TestWriteLibrarianState(t *testing.T) {
731731
err := writeLibrarianState(test.state, filePath)
732732
if test.wantErr {
733733
if err == nil {
734-
t.Errorf("writeLibrarianState() shoud fail")
734+
t.Fatalf("writeLibrarianState() shoud fail")
735735
}
736736

737737
if !strings.Contains(err.Error(), test.wantErrMsg) {

‎internal/github/github_test.go‎

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,9 @@ func TestGetRawContent(t *testing.T) {
105105

106106
if test.wantErr {
107107
if err == nil {
108-
t.Errorf("GetRawContent() err = nil, want error containing %q", test.wantErrSubstr)
109-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
108+
t.Fatalf("GetRawContent() err = nil, want error containing %q", test.wantErrSubstr)
109+
}
110+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
110111
t.Errorf("GetRawContent() err = %v, want error containing %q", err, test.wantErrSubstr)
111112
}
112113
} else {
@@ -220,8 +221,9 @@ func TestFetchGitHubRepoFromRemote(t *testing.T) {
220221

221222
if test.wantErr {
222223
if err == nil {
223-
t.Errorf("FetchGitHubRepoFromRemote() err = nil, want error containing %q", test.wantErrSubstr)
224-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
224+
t.Fatalf("FetchGitHubRepoFromRemote() err = nil, want error containing %q", test.wantErrSubstr)
225+
}
226+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
225227
t.Errorf("FetchGitHubRepoFromRemote() err = %v, want error containing %q", err, test.wantErrSubstr)
226228
}
227229
} else {
@@ -276,8 +278,9 @@ func TestParseURL(t *testing.T) {
276278

277279
if test.wantErr {
278280
if err == nil {
279-
t.Errorf("ParseURL() err = nil, want error containing %q", test.wantErrSubstr)
280-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
281+
t.Fatalf("ParseURL() err = nil, want error containing %q", test.wantErrSubstr)
282+
}
283+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
281284
t.Errorf("ParseURL() err = %v, want error containing %q", err, test.wantErrSubstr)
282285
}
283286
} else {
@@ -335,8 +338,9 @@ func TestParseSSHRemote(t *testing.T) {
335338
repo, err := parseSSHRemote(test.remote)
336339
if test.wantErr {
337340
if err == nil {
338-
t.Errorf("ParseSSHRemote() err = nil, want error containing %q", test.wantErrSubstr)
339-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
341+
t.Fatalf("ParseSSHRemote() err = nil, want error containing %q", test.wantErrSubstr)
342+
}
343+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
340344
t.Errorf("ParseSSHRemote() err = %v, want error containing %q", err, test.wantErrSubstr)
341345
}
342346
} else {
@@ -440,8 +444,9 @@ func TestCreatePullRequest(t *testing.T) {
440444

441445
if test.wantErr {
442446
if err == nil {
443-
t.Errorf("CreatePullRequest() err = nil, want error containing %q", test.wantErrSubstr)
444-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
447+
t.Fatalf("CreatePullRequest() err = nil, want error containing %q", test.wantErrSubstr)
448+
}
449+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
445450
t.Errorf("CreatePullRequest() err = %v, want error containing %q", err, test.wantErrSubstr)
446451
}
447452
} else if err != nil {
@@ -507,7 +512,7 @@ func TestAddLabelsToIssue(t *testing.T) {
507512

508513
if test.wantErr {
509514
if err == nil {
510-
t.Errorf("AddLabelsToIssue() should return an error")
515+
t.Fatal("AddLabelsToIssue() should return an error")
511516
}
512517
if !strings.Contains(err.Error(), test.wantErrSubstr) {
513518
t.Errorf("AddLabelsToIssue() err = %v, want error containing %q", err, test.wantErrSubstr)
@@ -582,7 +587,7 @@ func TestGetLabels(t *testing.T) {
582587

583588
if test.wantErr {
584589
if err == nil {
585-
t.Errorf("GetLabels() should return an error")
590+
t.Fatal("GetLabels() should return an error")
586591
}
587592
if !strings.Contains(err.Error(), test.wantErrSubstr) {
588593
t.Errorf("GetLabels() err = %v, want error containing %q", err, test.wantErrSubstr)
@@ -655,7 +660,7 @@ func TestReplaceLabels(t *testing.T) {
655660

656661
if test.wantErr {
657662
if err == nil {
658-
t.Errorf("ReplaceLabels() should return an error")
663+
t.Fatal("ReplaceLabels() should return an error")
659664
}
660665
if !strings.Contains(err.Error(), test.wantErrSubstr) {
661666
t.Errorf("ReplaceLabels() err = %v, want error containing %q", err, test.wantErrSubstr)
@@ -761,8 +766,9 @@ func TestSearchPullRequests(t *testing.T) {
761766

762767
if test.wantErr {
763768
if err == nil {
764-
t.Errorf("SearchPullRequests() err = nil, want error containing %q", test.wantErrSubstr)
765-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
769+
t.Fatalf("SearchPullRequests() err = nil, want error containing %q", test.wantErrSubstr)
770+
}
771+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
766772
t.Errorf("SearchPullRequests() err = %v, want error containing %q", err, test.wantErrSubstr)
767773
}
768774
} else if err != nil {
@@ -824,8 +830,9 @@ func TestGetPullRequest(t *testing.T) {
824830

825831
if test.wantErr {
826832
if err == nil {
827-
t.Errorf("GetPullRequest() err = nil, want error containing %q", test.wantErrSubstr)
828-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
833+
t.Fatalf("GetPullRequest() err = nil, want error containing %q", test.wantErrSubstr)
834+
}
835+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
829836
t.Errorf("GetPullRequest() err = %v, want error containing %q", err, test.wantErrSubstr)
830837
}
831838
} else if err != nil {
@@ -901,8 +908,9 @@ func TestCreateRelease(t *testing.T) {
901908

902909
if test.wantErr {
903910
if err == nil {
904-
t.Errorf("CreateRelease() err = nil, want error containing %q", test.wantErrSubstr)
905-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
911+
t.Fatalf("CreateRelease() err = nil, want error containing %q", test.wantErrSubstr)
912+
}
913+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
906914
t.Errorf("CreateRelease() err = %v, want error containing %q", err, test.wantErrSubstr)
907915
}
908916
} else if err != nil {
@@ -970,8 +978,9 @@ func TestCreateIssueComment(t *testing.T) {
970978

971979
if test.wantErr {
972980
if err == nil {
973-
t.Errorf("CreateComment() err = nil, want error containing %q", test.wantErrSubstr)
974-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
981+
t.Fatalf("CreateComment() err = nil, want error containing %q", test.wantErrSubstr)
982+
}
983+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
975984
t.Errorf("CreateComment() err = %v, want error containing %q", err, test.wantErrSubstr)
976985
}
977986
} else if err != nil {
@@ -1033,8 +1042,9 @@ func TestFindMergedPullRequestsWithPendingReleaseLabel(t *testing.T) {
10331042

10341043
if test.wantErr {
10351044
if err == nil {
1036-
t.Errorf("FindMergedPullRequestsWithPendingReleaseLabel() err = nil, want error containing %q", test.wantErrSubstr)
1037-
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
1045+
t.Fatalf("FindMergedPullRequestsWithPendingReleaseLabel() err = nil, want error containing %q", test.wantErrSubstr)
1046+
}
1047+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
10381048
t.Errorf("FindMergedPullRequestsWithPendingReleaseLabel() err = %v, want error containing %q", err, test.wantErrSubstr)
10391049
}
10401050
} else if err != nil {
@@ -1100,7 +1110,7 @@ func TestCreateTag(t *testing.T) {
11001110

11011111
if test.wantErr {
11021112
if err == nil {
1103-
t.Errorf("CreateTag() err = nil, expected error")
1113+
t.Fatal("CreateTag() err = nil, expected error")
11041114
}
11051115
} else if err != nil {
11061116
t.Errorf("CreateTag() err = %v, want nil", err)

‎internal/gitrepo/gitrepo_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,7 @@ func TestGetCommit(t *testing.T) {
662662
got, err := repo.GetCommit(commitHash)
663663
if test.wantErr {
664664
if err == nil {
665-
t.Error("GetCommit() should fail")
665+
t.Fatal("GetCommit() should fail")
666666
}
667667
if !strings.Contains(err.Error(), test.wantErrMsg) {
668668
t.Errorf("want error message %s, got %s", test.wantErrMsg, err.Error())
@@ -991,7 +991,7 @@ func TestGetCommitsForPathsSinceCommit(t *testing.T) {
991991

992992
if test.wantErr {
993993
if err == nil {
994-
t.Errorf("%s should return error", test.name)
994+
t.Fatalf("%s should return error", test.name)
995995
}
996996
if !strings.Contains(err.Error(), test.wantErrPhrase) {
997997
t.Errorf("GetCommitsForPathsSinceCommit() returned error %q, want to contain %q", err.Error(), test.wantErrPhrase)
@@ -1053,7 +1053,7 @@ func TestGetCommitsForPathsSinceTag(t *testing.T) {
10531053

10541054
if test.wantErr {
10551055
if err == nil {
1056-
t.Errorf("%s should return error", test.name)
1056+
t.Fatalf("%s should return error", test.name)
10571057
}
10581058
if !strings.Contains(err.Error(), test.wantErrPhrase) {
10591059
t.Errorf("GetCommitsForPathsSinceTag() returned error %q, want to contain %q", err.Error(), test.wantErrPhrase)
@@ -1099,7 +1099,7 @@ func TestCreateBranchAndCheckout(t *testing.T) {
10991099
err := repo.CreateBranchAndCheckout(test.branchName)
11001100
if test.wantErr {
11011101
if err == nil {
1102-
t.Errorf("%s should return error", test.name)
1102+
t.Fatalf("%s should return error", test.name)
11031103
}
11041104
if !strings.Contains(err.Error(), test.wantErrPhrase) {
11051105
t.Errorf("CreateBranchAndCheckout() returned error %q, want to contain %q", err.Error(), test.wantErrPhrase)

0 commit comments

Comments
 (0)