Skip to content

Commit 12105b8

Browse files
efarrerejoffe
efarrer
authored and
ejoffe
committed
More rigorous test framework and fix tests
The unit tests didn't rigorously test expectations. This led to tests which were broken but passed anyway. For example for the git Mocks you could add expectations for X, Y, and Z and if only X and Y were called the tests would still pass. For the github mocks the same issue existed but also they would pass if X and Y expectations were set but X and Y were called multiple times. This change fixes both of these issues and fixes the unit tests so they are no longer broken.
1 parent bb8cf6c commit 12105b8

File tree

4 files changed

+90
-16
lines changed

4 files changed

+90
-16
lines changed

‎config/config_parser/config_parser_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,5 @@ func TestGitHubRemoteSource(t *testing.T) {
9191
source := NewGitHubRemoteSource(&actual, mock)
9292
source.Load(nil)
9393
assert.Equal(t, expect, actual)
94+
mock.ExpectationsMet()
9495
}

‎git/mockgit/mockgit.go

+11-2
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ func (m *Mock) GitWithEditor(args string, output *string, editorCmd string) erro
2323
func (m *Mock) Git(args string, output *string) error {
2424
fmt.Printf("CMD: git %s\n", args)
2525

26-
m.assert.NotEmpty(m.expectedCmd)
26+
m.assert.NotEmpty(m.expectedCmd, fmt.Sprintf("Unexpected command: git %s\n", args))
2727

2828
expected := m.expectedCmd[0]
2929
actual := "git " + args
@@ -42,6 +42,11 @@ func (m *Mock) Git(args string, output *string) error {
4242
return nil
4343
}
4444

45+
func (m *Mock) ExpectationsMet() {
46+
m.assert.Empty(m.expectedCmd, fmt.Sprintf("expected additional git commands: %v", m.expectedCmd))
47+
m.assert.Empty(m.response, fmt.Sprintf("expected additional git responses: %v", m.response))
48+
}
49+
4550
func (m *Mock) MustGit(argStr string, output *string) {
4651
err := m.Git(argStr, output)
4752
if err != nil {
@@ -73,8 +78,12 @@ func (m *Mock) ExpectLogAndRespond(commits []*git.Commit) {
7378
m.expect("git log --format=medium --no-color origin/master..HEAD").commitRespond(commits)
7479
}
7580

76-
func (m *Mock) ExpectPushCommits(commits []*git.Commit) {
81+
func (m *Mock) ExpectStatus() {
7782
m.expect("git status --porcelain --untracked-files=no").commitRespond(nil)
83+
}
84+
85+
func (m *Mock) ExpectPushCommits(commits []*git.Commit) {
86+
m.ExpectStatus()
7887

7988
var refNames []string
8089
for _, c := range commits {

‎github/mockclient/mockclient.go

+17-5
Original file line numberDiff line numberDiff line change
@@ -208,15 +208,27 @@ func (c *MockClient) verifyExpectation(actual expectation) {
208208
c.expectMutex.Lock()
209209
defer c.expectMutex.Unlock()
210210

211-
for _, expected := range c.expect {
212-
if reflect.DeepEqual(expected, actual) {
211+
for i := 0; i != len(c.expect); i++ {
212+
if reflect.DeepEqual(c.expect[i], actual) {
213+
// Zero out the expectation once it has been met
214+
c.expect[i] = expectation{}
213215
return
214216
}
215217
}
216218

217-
expected := c.expect[0]
218-
c.assert.Equal(expected, actual)
219-
c.expect = c.expect[1:]
219+
c.assert.FailNowf("verifyExpectations", "Unexpected github command: %v\n", actual)
220+
}
221+
222+
func (c *MockClient) ExpectationsMet() {
223+
c.expectMutex.Lock()
224+
defer c.expectMutex.Unlock()
225+
226+
// Note that above the expectations are zeroed (not removed)
227+
for _, expected := range c.expect {
228+
if expected.op != "" {
229+
c.assert.FailNowf("ExpectationsMet", "expected additional github commands: %#v", expected)
230+
}
231+
}
220232
}
221233

222234
type operation string

‎spr/spr_test.go

+61-9
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ func TestSPRBasicFlowFourCommitsQueue(t *testing.T) {
7070
githubmock.ExpectGetInfo()
7171
s.StatusPullRequests(ctx)
7272
assert.Equal("pull request stack is empty\n", output.String())
73+
gitmock.ExpectationsMet()
74+
githubmock.ExpectationsMet()
7375
output.Reset()
7476

7577
// 'git spr update' :: UpdatePullRequest :: commits=[c1]
@@ -85,6 +87,8 @@ func TestSPRBasicFlowFourCommitsQueue(t *testing.T) {
8587
s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil)
8688
fmt.Printf("OUT: %s\n", output.String())
8789
assert.Equal("[vvvv] 1 : test commit 1\n", output.String())
90+
gitmock.ExpectationsMet()
91+
githubmock.ExpectationsMet()
8892
output.Reset()
8993

9094
// 'git spr update' :: UpdatePullRequest :: commits=[c1, c2]
@@ -104,6 +108,8 @@ func TestSPRBasicFlowFourCommitsQueue(t *testing.T) {
104108
assert.Equal("warning: not updating reviewers for PR #1", lines[0])
105109
assert.Equal("[vvvv] 1 : test commit 2", lines[1])
106110
assert.Equal("[vvvv] 1 : test commit 1", lines[2])
111+
gitmock.ExpectationsMet()
112+
githubmock.ExpectationsMet()
107113
output.Reset()
108114

109115
// 'git spr update' :: UpdatePullRequest :: commits=[c1, c2, c3, c4]
@@ -137,6 +143,8 @@ func TestSPRBasicFlowFourCommitsQueue(t *testing.T) {
137143
"[vvvv] 1 : test commit 2",
138144
"[vvvv] 1 : test commit 1",
139145
}, lines[:6])
146+
gitmock.ExpectationsMet()
147+
githubmock.ExpectationsMet()
140148
output.Reset()
141149

142150
// 'git spr merge' :: MergePullRequest :: commits=[a1, a2]
@@ -145,23 +153,28 @@ func TestSPRBasicFlowFourCommitsQueue(t *testing.T) {
145153
githubmock.ExpectMergePullRequest(c2, genclient.PullRequestMergeMethod_REBASE)
146154
githubmock.ExpectCommentPullRequest(c1)
147155
githubmock.ExpectClosePullRequest(c1)
148-
githubmock.ExpectCommentPullRequest(c2)
149-
githubmock.ExpectClosePullRequest(c2)
150156
count := uint(2)
151157
s.MergePullRequests(ctx, &count)
152158
lines = strings.Split(output.String(), "\n")
153159
assert.Equal("MERGED 1 : test commit 1", lines[0])
154160
assert.Equal("MERGED 1 : test commit 2", lines[1])
155161
fmt.Printf("OUT: %s\n", output.String())
162+
gitmock.ExpectationsMet()
163+
githubmock.ExpectationsMet()
156164
output.Reset()
157165

158166
githubmock.Info.PullRequests = githubmock.Info.PullRequests[1:]
159167
githubmock.Info.PullRequests[0].Merged = false
160168
githubmock.Info.PullRequests[0].Commits = append(githubmock.Info.PullRequests[0].Commits, c1, c2)
169+
githubmock.ExpectGetInfo()
170+
githubmock.ExpectUpdatePullRequest(c2, nil)
171+
githubmock.ExpectUpdatePullRequest(c3, &c2)
172+
githubmock.ExpectUpdatePullRequest(c4, &c3)
173+
githubmock.ExpectGetInfo()
161174

162175
gitmock.ExpectFetch()
163176
gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1})
164-
gitmock.ExpectPushCommits([]*git.Commit{&c2, &c3, &c4})
177+
gitmock.ExpectStatus()
165178

166179
s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil)
167180
lines = strings.Split(output.String(), "\n")
@@ -174,12 +187,12 @@ func TestSPRBasicFlowFourCommitsQueue(t *testing.T) {
174187
"[vvvv] 1 : test commit 3",
175188
"[vvvv] ! 1 : test commit 2",
176189
}, lines[:6])
190+
gitmock.ExpectationsMet()
191+
githubmock.ExpectationsMet()
177192
output.Reset()
178193

179194
// 'git spr merge' :: MergePullRequest :: commits=[a2, a3, a4]
180-
gitmock.ExpectLocalBranch("master")
181195
githubmock.ExpectGetInfo()
182-
gitmock.ExpectLocalBranch("master")
183196
githubmock.ExpectUpdatePullRequest(c4, nil)
184197
githubmock.ExpectMergePullRequest(c4, genclient.PullRequestMergeMethod_REBASE)
185198

@@ -196,6 +209,8 @@ func TestSPRBasicFlowFourCommitsQueue(t *testing.T) {
196209
assert.Equal("MERGED 1 : test commit 3", lines[1])
197210
assert.Equal("MERGED 1 : test commit 4", lines[2])
198211
fmt.Printf("OUT: %s\n", output.String())
212+
gitmock.ExpectationsMet()
213+
githubmock.ExpectationsMet()
199214
output.Reset()
200215
}
201216

@@ -229,6 +244,8 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
229244
githubmock.ExpectGetInfo()
230245
s.StatusPullRequests(ctx)
231246
assert.Equal("pull request stack is empty\n", output.String())
247+
gitmock.ExpectationsMet()
248+
githubmock.ExpectationsMet()
232249
output.Reset()
233250

234251
// 'git spr update' :: UpdatePullRequest :: commits=[c1]
@@ -244,6 +261,8 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
244261
s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil)
245262
fmt.Printf("OUT: %s\n", output.String())
246263
assert.Equal("[vvvv] 1 : test commit 1\n", output.String())
264+
gitmock.ExpectationsMet()
265+
githubmock.ExpectationsMet()
247266
output.Reset()
248267

249268
// 'git spr update' :: UpdatePullRequest :: commits=[c1, c2]
@@ -263,6 +282,8 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
263282
assert.Equal("warning: not updating reviewers for PR #1", lines[0])
264283
assert.Equal("[vvvv] 1 : test commit 2", lines[1])
265284
assert.Equal("[vvvv] 1 : test commit 1", lines[2])
285+
gitmock.ExpectationsMet()
286+
githubmock.ExpectationsMet()
266287
output.Reset()
267288

268289
// 'git spr update' :: UpdatePullRequest :: commits=[c1, c2, c3, c4]
@@ -296,12 +317,12 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
296317
"[vvvv] 1 : test commit 2",
297318
"[vvvv] 1 : test commit 1",
298319
}, lines[:6])
320+
gitmock.ExpectationsMet()
321+
githubmock.ExpectationsMet()
299322
output.Reset()
300323

301324
// 'git spr merge' :: MergePullRequest :: commits=[a1, a2, a3, a4]
302-
gitmock.ExpectLocalBranch("master")
303325
githubmock.ExpectGetInfo()
304-
gitmock.ExpectLocalBranch("master")
305326
githubmock.ExpectUpdatePullRequest(c4, nil)
306327
githubmock.ExpectMergePullRequest(c4, genclient.PullRequestMergeMethod_REBASE)
307328
githubmock.ExpectCommentPullRequest(c1)
@@ -317,6 +338,8 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
317338
assert.Equal("MERGED 1 : test commit 3", lines[2])
318339
assert.Equal("MERGED 1 : test commit 4", lines[3])
319340
fmt.Printf("OUT: %s\n", output.String())
341+
gitmock.ExpectationsMet()
342+
githubmock.ExpectationsMet()
320343
output.Reset()
321344
}
322345

@@ -375,6 +398,8 @@ func TestSPRMergeCount(t *testing.T) {
375398
"[vvvv] 1 : test commit 2",
376399
"[vvvv] 1 : test commit 1",
377400
}, lines[:4])
401+
gitmock.ExpectationsMet()
402+
githubmock.ExpectationsMet()
378403
output.Reset()
379404

380405
// 'git spr merge --count 2' :: MergePullRequest :: commits=[a1, a2, a3, a4]
@@ -388,6 +413,8 @@ func TestSPRMergeCount(t *testing.T) {
388413
assert.Equal("MERGED 1 : test commit 1", lines[0])
389414
assert.Equal("MERGED 1 : test commit 2", lines[1])
390415
fmt.Printf("OUT: %s\n", output.String())
416+
gitmock.ExpectationsMet()
417+
githubmock.ExpectationsMet()
391418
output.Reset()
392419
}
393420

@@ -411,6 +438,8 @@ func TestSPRAmendCommit(t *testing.T) {
411438
githubmock.ExpectGetInfo()
412439
s.StatusPullRequests(ctx)
413440
assert.Equal("pull request stack is empty\n", output.String())
441+
gitmock.ExpectationsMet()
442+
githubmock.ExpectationsMet()
414443
output.Reset()
415444

416445
// 'git spr update' :: UpdatePullRequest :: commits=[c1, c2]
@@ -428,6 +457,8 @@ func TestSPRAmendCommit(t *testing.T) {
428457
lines := strings.Split(output.String(), "\n")
429458
assert.Equal("[vvvv] 1 : test commit 2", lines[0])
430459
assert.Equal("[vvvv] 1 : test commit 1", lines[1])
460+
gitmock.ExpectationsMet()
461+
githubmock.ExpectationsMet()
431462
output.Reset()
432463

433464
// amend commit c2
@@ -445,6 +476,8 @@ func TestSPRAmendCommit(t *testing.T) {
445476
fmt.Printf("OUT: %s\n", output.String())
446477
assert.Equal("[vvvv] 1 : test commit 2", lines[0])
447478
assert.Equal("[vvvv] 1 : test commit 1", lines[1])
479+
gitmock.ExpectationsMet()
480+
githubmock.ExpectationsMet()
448481
output.Reset()
449482

450483
// amend commit c1
@@ -463,6 +496,8 @@ func TestSPRAmendCommit(t *testing.T) {
463496
fmt.Printf("OUT: %s\n", output.String())
464497
assert.Equal("[vvvv] 1 : test commit 2", lines[0])
465498
assert.Equal("[vvvv] 1 : test commit 1", lines[1])
499+
gitmock.ExpectationsMet()
500+
githubmock.ExpectationsMet()
466501
output.Reset()
467502

468503
// 'git spr merge' :: MergePullRequest :: commits=[a1, a2]
@@ -471,13 +506,13 @@ func TestSPRAmendCommit(t *testing.T) {
471506
githubmock.ExpectMergePullRequest(c2, genclient.PullRequestMergeMethod_REBASE)
472507
githubmock.ExpectCommentPullRequest(c1)
473508
githubmock.ExpectClosePullRequest(c1)
474-
githubmock.ExpectCommentPullRequest(c2)
475-
githubmock.ExpectClosePullRequest(c2)
476509
s.MergePullRequests(ctx, nil)
477510
lines = strings.Split(output.String(), "\n")
478511
assert.Equal("MERGED 1 : test commit 1", lines[0])
479512
assert.Equal("MERGED 1 : test commit 2", lines[1])
480513
fmt.Printf("OUT: %s\n", output.String())
514+
gitmock.ExpectationsMet()
515+
githubmock.ExpectationsMet()
481516
output.Reset()
482517
}
483518

@@ -516,6 +551,8 @@ func TestSPRReorderCommit(t *testing.T) {
516551
githubmock.ExpectGetInfo()
517552
s.StatusPullRequests(ctx)
518553
assert.Equal("pull request stack is empty\n", output.String())
554+
gitmock.ExpectationsMet()
555+
githubmock.ExpectationsMet()
519556
output.Reset()
520557

521558
// 'git spr update' :: UpdatePullRequest :: commits=[c1, c2, c3, c4]
@@ -539,6 +576,8 @@ func TestSPRReorderCommit(t *testing.T) {
539576
assert.Equal("[vvvv] 1 : test commit 3", lines[1])
540577
assert.Equal("[vvvv] 1 : test commit 2", lines[2])
541578
assert.Equal("[vvvv] 1 : test commit 1", lines[3])
579+
gitmock.ExpectationsMet()
580+
githubmock.ExpectationsMet()
542581
output.Reset()
543582

544583
// 'git spr update' :: UpdatePullRequest :: commits=[c2, c4, c1, c3]
@@ -568,6 +607,8 @@ func TestSPRReorderCommit(t *testing.T) {
568607
//assert.Equal("[vvvv] 1 : test commit 1", lines[1])
569608
//assert.Equal("[vvvv] 1 : test commit 4", lines[2])
570609
//assert.Equal("[vvvv] 1 : test commit 2", lines[3])
610+
gitmock.ExpectationsMet()
611+
githubmock.ExpectationsMet()
571612
output.Reset()
572613

573614
// 'git spr update' :: UpdatePullRequest :: commits=[c5, c1, c2, c3, c4]
@@ -600,6 +641,8 @@ func TestSPRReorderCommit(t *testing.T) {
600641
//assert.Equal("[vvvv] 1 : test commit 3", lines[2])
601642
//assert.Equal("[vvvv] 1 : test commit 2", lines[3])
602643
//assert.Equal("[vvvv] 1 : test commit 1", lines[4])
644+
gitmock.ExpectationsMet()
645+
githubmock.ExpectationsMet()
603646
output.Reset()
604647

605648
// TODO : add a call to merge and check merge order
@@ -635,6 +678,8 @@ func TestSPRDeleteCommit(t *testing.T) {
635678
githubmock.ExpectGetInfo()
636679
s.StatusPullRequests(ctx)
637680
assert.Equal("pull request stack is empty\n", output.String())
681+
gitmock.ExpectationsMet()
682+
githubmock.ExpectationsMet()
638683
output.Reset()
639684

640685
// 'git spr update' :: UpdatePullRequest :: commits=[c1, c2, c3, c4]
@@ -659,6 +704,8 @@ func TestSPRDeleteCommit(t *testing.T) {
659704
assert.Equal("[vvvv] 1 : test commit 3", lines[1])
660705
assert.Equal("[vvvv] 1 : test commit 2", lines[2])
661706
assert.Equal("[vvvv] 1 : test commit 1", lines[3])
707+
gitmock.ExpectationsMet()
708+
githubmock.ExpectationsMet()
662709
output.Reset()
663710

664711
// 'git spr update' :: UpdatePullRequest :: commits=[c2, c4, c1, c3]
@@ -684,6 +731,8 @@ func TestSPRDeleteCommit(t *testing.T) {
684731
//assert.Equal("[vvvv] 1 : test commit 1", lines[1])
685732
//assert.Equal("[vvvv] 1 : test commit 4", lines[2])
686733
//assert.Equal("[vvvv] 1 : test commit 2", lines[3])
734+
gitmock.ExpectationsMet()
735+
githubmock.ExpectationsMet()
687736
output.Reset()
688737

689738
// TODO : add a call to merge and check merge order
@@ -753,18 +802,21 @@ func TestAmendInvalidInput(t *testing.T) {
753802
input.WriteString("a")
754803
s.AmendCommit(ctx)
755804
assert.Equal(" 1 : 00000001 : test commit 1\nCommit to amend (1): Invalid input\n", output.String())
805+
gitmock.ExpectationsMet()
756806
output.Reset()
757807

758808
gitmock.ExpectLogAndRespond([]*git.Commit{&c1})
759809
input.WriteString("0")
760810
s.AmendCommit(ctx)
761811
assert.Equal(" 1 : 00000001 : test commit 1\nCommit to amend (1): Invalid input\n", output.String())
812+
gitmock.ExpectationsMet()
762813
output.Reset()
763814

764815
gitmock.ExpectLogAndRespond([]*git.Commit{&c1})
765816
input.WriteString("2")
766817
s.AmendCommit(ctx)
767818
assert.Equal(" 1 : 00000001 : test commit 1\nCommit to amend (1): Invalid input\n", output.String())
819+
gitmock.ExpectationsMet()
768820
output.Reset()
769821
}
770822

0 commit comments

Comments
 (0)