Skip to content
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

BS: Add fast recovery unit test #2694

Merged
merged 2 commits into from
May 17, 2019

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented May 16, 2019

The added tests check that the fast recovery mechanism works as
expected.

fixes #2685


This change is Reviewable

@oncilla oncilla added the BS label May 16, 2019
@oncilla oncilla added this to the Q2S2 milestone May 16, 2019
@oncilla oncilla requested a review from lukedirtwalker May 16, 2019 12:17
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @oncilla)


go/beacon_srv/internal/beaconing/originator_test.go, line 144 at r1 (raw file):

		// 3. Run where no beacon is sent. -> no call
		// 4. Run where beacons are sent on all interfaces. -> 2 calls
		conn.EXPECT().WriteTo(gomock.Any(), gomock.Any()).Times(5).DoAndReturn(

See comment below, maybe this could be simplified.


go/beacon_srv/internal/beaconing/propagator_test.go, line 245 at r1 (raw file):

		}
		g := graph.NewDefaultGraph(mctrl)
		// We call run 4 times in this test

according to the explanation below and the code I wouldn't expect a call in run 3. Why is there one?


go/beacon_srv/internal/beaconing/propagator_test.go, line 260 at r1 (raw file):

		// 3. Run where no beacon is sent. -> no call
		// 4. Run where beacons are sent on all interfaces. -> 2 calls
		conn.EXPECT().WriteTo(gomock.Any(), gomock.Any()).Times(5).DoAndReturn(

you could do this more explicit and concise I think:

firstCall := conn.EXPECT().WriteTo(gomock.Any(), gomock.Any())
secondCall := conn.EXPECT().WriteTo(gomock.Any(), gomock.Any()).After(firstCall).Return(errors.New("fail"))
conn.EXPECT().WriteTo(gomock.Any(), gomock.Any()).After(secondCall).Times(3)

(didn't test it though)


go/beacon_srv/internal/beaconing/propagator_test.go, line 276 at r1 (raw file):

		// Second run. One write expected.
		p.Run(nil)
		// Third run. No write expected

hm this can't really be verified, but the total is good enough -ish

oncilla added 2 commits May 17, 2019 14:59
The added tests check that the fast recovery mechanism works as
expected.

fixes scionproto#2685
@oncilla oncilla force-pushed the pub-bs-fast-recovery-unit branch from 91b8af4 to 6eb4c35 Compare May 17, 2019 13:00
Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)


go/beacon_srv/internal/beaconing/originator_test.go, line 144 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

See comment below, maybe this could be simplified.

Done.


go/beacon_srv/internal/beaconing/propagator_test.go, line 245 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

according to the explanation below and the code I wouldn't expect a call in run 3. Why is there one?

Done.


go/beacon_srv/internal/beaconing/propagator_test.go, line 260 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

you could do this more explicit and concise I think:

firstCall := conn.EXPECT().WriteTo(gomock.Any(), gomock.Any())
secondCall := conn.EXPECT().WriteTo(gomock.Any(), gomock.Any()).After(firstCall).Return(errors.New("fail"))
conn.EXPECT().WriteTo(gomock.Any(), gomock.Any()).After(secondCall).Times(3)

(didn't test it though)

Nice.


go/beacon_srv/internal/beaconing/propagator_test.go, line 276 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

hm this can't really be verified, but the total is good enough -ish

Sure, but its good enough I think.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@oncilla oncilla merged commit e3f24c1 into scionproto:master May 17, 2019
@oncilla oncilla deleted the pub-bs-fast-recovery-unit branch May 17, 2019 13:47
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.

BS: Add unit test for fast recovery feature.
2 participants