-
Notifications
You must be signed in to change notification settings - Fork 163
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
pathmgr: fix flaky unit test #2159
Conversation
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.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)
go/lib/pathmgr/pathmgr_test.go, line 209 at r1 (raw file):
).MinTimes(1), ) pr := New(sd, Timers{ErrorRefire: getDuration(1)}, nil)
We can get rid of all timing problems I think by using a channel:
func TestWatchPolling(t *testing.T) {
src := xtest.MustParseIA("1-ff00:0:111")
dst := xtest.MustParseIA("1-ff00:0:110")
Convey("Given a path manager", t, func() {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
sd := mock_sciond.NewMockConnector(ctrl)
ready := make(chan struct{})
gomock.InOrder(
sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).Return(
buildSDAnswer(), nil,
),
sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).DoAndReturn(
func(_ context.Context, _, _ addr.IA, _ uint16,
_ sciond.PathReqFlags) (*sciond.PathReply, error) {
defer close(ready)
return buildSDAnswer(
"1-ff00:0:111#105 1-ff00:0:130#1002 1-ff00:0:130#1004 1-ff00:0:110#2",
), nil
},
).MinTimes(1),
)
pr := New(sd, Timers{ErrorRefire: time.Microsecond}, nil)
Convey("and adding a watch that retrieves zero paths", func() {
sp, err := pr.Watch(context.Background(), src, dst)
xtest.FailOnErr(t, err)
Convey("there are 0 paths currently available", func() {
So(len(sp.Load().APS), ShouldEqual, 0)
Convey("and waiting until new paths are fetched", func() {
<-ready
So(len(sp.Load().APS), ShouldEqual, 1)
})
})
})
})
}
go/lib/pathmgr/pathmgr_test.go, line 256 at r1 (raw file):
So(len(sp.Load().APS), ShouldEqual, 0) Convey("and waiting for 4 time units grabs 1 path that is not filtered", func() { time.Sleep(getDuration(4))
Here we can probably also use the channel trick?
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)
go/lib/pathmgr/pathmgr_test.go, line 209 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
We can get rid of all timing problems I think by using a channel:
func TestWatchPolling(t *testing.T) { src := xtest.MustParseIA("1-ff00:0:111") dst := xtest.MustParseIA("1-ff00:0:110") Convey("Given a path manager", t, func() { ctrl := gomock.NewController(t) defer ctrl.Finish() sd := mock_sciond.NewMockConnector(ctrl) ready := make(chan struct{}) gomock.InOrder( sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).Return( buildSDAnswer(), nil, ), sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).DoAndReturn( func(_ context.Context, _, _ addr.IA, _ uint16, _ sciond.PathReqFlags) (*sciond.PathReply, error) { defer close(ready) return buildSDAnswer( "1-ff00:0:111#105 1-ff00:0:130#1002 1-ff00:0:130#1004 1-ff00:0:110#2", ), nil }, ).MinTimes(1), ) pr := New(sd, Timers{ErrorRefire: time.Microsecond}, nil) Convey("and adding a watch that retrieves zero paths", func() { sp, err := pr.Watch(context.Background(), src, dst) xtest.FailOnErr(t, err) Convey("there are 0 paths currently available", func() { So(len(sp.Load().APS), ShouldEqual, 0) Convey("and waiting until new paths are fetched", func() { <-ready So(len(sp.Load().APS), ShouldEqual, 1) }) }) }) }) }
The channel solution like this is also not 100% bullet proof. We could make it work with by closing the ready channel only on the second call to Paths
. But as discussed offline let's go with the simple solution of using ErrorRefire: time.Microsecond
and 10 or 15 ms sleep below.
go/lib/pathmgr/pathmgr_test.go, line 256 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Here we can probably also use the channel trick?
Ditto as above.
Also make test run faster by deleting deprecated sleeps.
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.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)
go/lib/pathmgr/pathmgr_test.go, line 209 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
The channel solution like this is also not 100% bullet proof. We could make it work with by closing the ready channel only on the second call to
Paths
. But as discussed offline let's go with the simple solution of usingErrorRefire: time.Microsecond
and 10 or 15 ms sleep below.
Done.
go/lib/pathmgr/pathmgr_test.go, line 256 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Ditto as above.
Done.
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.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
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.
Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
Also make test run faster by deleting deprecated sleeps.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)