Remove SCIOND API mock #3014

karampok · 2019-08-20T12:54:19Z

Replace the manual generated file mock.go to use the autogenerated
mock_sciond

This fixes the #2377 issue

This change is

oncilla

Reviewed 4 of 5 files at r1.
Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @karampok and @oncilla)

go/lib/pathmgr/pathmgr_test.go, line 56 at r1 (raw file):

		g := graph.NewDefaultGraph(ctrl)

		Convey("Query immediately, get paths", func() {

While here replace this with regular go sub tests. (here and the other tests as well)

i.e t.Run("Query immediately, get paths", func(t *testing.T) { ... code below })

go/lib/pathmgr/pathmgr_test.go, line 58 at r1 (raw file):

		Convey("Query immediately, get paths", func() {
			sd.EXPECT().Paths(gomock.Any(), dstIA, srcIA, gomock.Any(), gomock.Any()).Return(
				buildGAnswer(srcIA.String(), dstIA.String(), g), nil,

use buildSDAnswer instead:
buildSDAnswer("1-ff00:0:133#1019 1-ff00:0:132#1910 1-ff00:0:132#1916 1-ff00:0:131#1619")

go/lib/pathmgr/pathmgr_test.go, line 61 at r1 (raw file):

			).AnyTimes()
			aps := pm.Query(context.Background(), srcIA, dstIA, sciond.PathReqFlags{})
			SoMsg("aps len", len(aps), ShouldEqual, 1)

replace this by assert.Len(t, 1, len(aps))

go/lib/pathmgr/pathmgr_test.go, line 62 at r1 (raw file):

			aps := pm.Query(context.Background(), srcIA, dstIA, sciond.PathReqFlags{})
			SoMsg("aps len", len(aps), ShouldEqual, 1)
			SoMsg("path", getPathStrings(aps), ShouldContain,

replace by assert.Contains(t, getPathStrings(aps), "... ")

go/lib/pathmgr/pathmgr_test.go, line 67 at r1 (raw file):

		})

		Convey("Then query again and get new paths", func() {

ditto

go/lib/pathmgr/util_test.go, line 34 at r1 (raw file):

// buildGAnswer does not guarantee to represent a consistent snapshot of the SCION
// network if the backing multigraph is modified while Paths is running.
func buildGAnswer(src, dst string, g *graph.Graph) *sciond.PathReply {

The same can be achieved with buildSDAnswer

oncilla

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @karampok)

go/lib/pathmgr/pathmgr_test.go, line 53 at r2 (raw file):

	dstIA := xtest.MustParseIA("1-ff00:0:131")

	t.Run("Query immediately, get paths", func(t *testing.T) {

Ah, I mislead you.

Judging from the sub tests name, they should actually not be sub tests and simply run sequentially.
i.e. just drop the t.Run -> tests should still pass
keep the sub test name as comment

go/lib/pathmgr/pathmgr_test.go, line 92 at r2 (raw file):

func TestQueryFilter(t *testing.T) {
	t.Log("Query with policy filter")

As an idea, the test could also be written as below.
But this is fine, leave it as is if you want

func TestQueryFilterA(t *testing.T) {
	t.Log("Query with policy filter")

	srcIA := xtest.MustParseIA("1-ff00:0:133")
	dstIA := xtest.MustParseIA("1-ff00:0:131")
	paths := []string{
		"1-ff00:0:133#1019 1-ff00:0:132#1910 " + "1-ff00:0:132#1916 1-ff00:0:131#1619",
	}
	tests := map[string]struct {
		Policy       func() *pathpol.Policy
		ShouldFilter bool
	}{
		"Hop does not exist in paths, default deny": {
			Policy: func() *pathpol.Policy {
				pp, err := pathpol.HopPredicateFromString("0-0#0")
				require.NoError(t, err)
				policy := &pathpol.Policy{ACL: &pathpol.ACL{Entries: []*pathpol.ACLEntry{
					{Action: pathpol.Allow, Rule: pp},
					denyEntry,
				}}}
				return policy
			},
		},
		"Hop does not exist paths, default allow": {
			Policy: func() *pathpol.Policy {
				pp, err := pathpol.HopPredicateFromString("1-ff00:0:134#1910")
				require.NoError(t, err)
				policy := &pathpol.Policy{ACL: &pathpol.ACL{Entries: []*pathpol.ACLEntry{
					{Action: pathpol.Allow, Rule: pp},
					allowEntry,
				}}}
				return policy
			},
		},
		"Hop exists in paths, default deny ": {
			Policy: func() *pathpol.Policy {
				pp, err := pathpol.HopPredicateFromString("1-ff00:0:132#1910")
				require.NoError(t, err)
				policy := &pathpol.Policy{ACL: &pathpol.ACL{Entries: []*pathpol.ACLEntry{
					{Action: pathpol.Deny, Rule: pp},
					denyEntry,
				}}}
				return policy
			},
			ShouldFilter: true,
		},
	}
	for name, test := range tests {
		t.Run(name, func(t *testing.T) {
			ctrl := gomock.NewController(t)
			defer ctrl.Finish()
			sd := mock_sciond.NewMockConnector(ctrl)
			pm := New(sd, Timers{})
			sd.EXPECT().Paths(gomock.Any(), dstIA, srcIA, gomock.Any(), gomock.Any()).Return(
				buildSDAnswer(paths...), nil,
			).AnyTimes()
			aps := pm.QueryFilter(context.Background(), srcIA, dstIA, test.Policy())
			if test.ShouldFilter {
				assert.Len(t, aps, 0, "no path should remain")
			} else {
				assert.Len(t, aps, 1, "only one path should remain")
				assert.ElementsMatch(t, getPathStrings(aps), paths)
			}
		})
	}
}

go/lib/pathmgr/pathmgr_test.go, line 112 at r2 (raw file):

	t.Run("Hop does not exist in paths, default deny", func(t *testing.T) {
		pp, err := pathpol.HopPredicateFromString("0-0#0")
		xtest.FailOnErr(t, err)

replace with require.NoError(t, err) here and everywhere else.

go/lib/pathmgr/pathmgr_test.go, line 163 at r2 (raw file):

	paths := []string{
		fmt.Sprintf("%s#1019 1-ff00:0:122#1910 1-ff00:0:122#1916 %s#1619",
			srcIA.String(), dstIA.String()),

no need for .String Sprintf will call the String method internally.

go/lib/pathmgr/pathmgr_test.go, line 310 at r2 (raw file):

	// are in error recovery mode, and the aggressive timer is used.
	// We actually test that the mock .{Revnotifications,Paths} functions are
	// being called within a 5 sec time period. It will fail with "missing

s/a 5 sec time period/5 time units/

go/lib/pathmgr/pathmgr_test.go, line 325 at r2 (raw file):

}

func TestRevoke(t *testing.T) {

What about:

func TestRevoke(t *testing.T) {
	t.Log("Given a path manager and a watch that")

	src := xtest.MustParseIA("1-ff00:0:111")
	dst := xtest.MustParseIA("1-ff00:0:110")
	paths := []string{
		"1-ff00:0:111#105 1-ff00:0:130#1002 1-ff00:0:130#1004 1-ff00:0:110#2",
		"1-ff00:0:111#104 1-ff00:0:120#5 1-ff00:0:120#6 1-ff00:0:110#1",
	}
	tests := map[string]struct {
		Paths         []string
		RevReply      *sciond.RevReply
		RevReplyError error
		Revocation    *path_mgmt.SignedRevInfo
		Remaining     int
	}{
		"retrieves one path revoking an IFID that matches the path": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevValid},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  0,
		},

		"retrieves one path revoking an IFID that does not match the path": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevValid},
			Revocation: newTestRev(t, "2-ff00:0:1#1"),
			Remaining:  1,
		},
		"tries to revoke an IFID, but SCIOND encounters an error": {
			Paths:         paths[:1],
			RevReplyError: errors.New("some error"),
			Revocation:    newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:     1,
		},
		"tries to revoke an IFID, but the revocation is invalid": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevInvalid},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  1,
		},
		"tries to revoke an IFID, but the revocation is stale": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevStale},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  1,
		},
		"tries to revoke an IFID, but the revocation is unknown": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevUnknown},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  0,
		},
		"retrieves two paths and revokes an IFID that matches one path": {
			Paths:      paths,
			RevReply:   &sciond.RevReply{Result: sciond.RevValid},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  1,
		},
	}
	for name, test := range tests {
		t.Run(name, func(t *testing.T) {
			ctrl := gomock.NewController(t)
			defer ctrl.Finish()
			sd := mock_sciond.NewMockConnector(ctrl)
			pr := New(sd, Timers{})

			sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).Return(
				buildSDAnswer(test.Paths...), nil,
			)
			sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).Return(
				buildSDAnswer(), nil,
			).AnyTimes()
			sp, err := pr.Watch(context.Background(), src, dst)
			require.NoError(t, err)
			sd.EXPECT().RevNotification(gomock.Any(), gomock.Any()).Return(
				test.RevReply, test.RevReplyError,
			)
			pr.Revoke(context.Background(), test.Revocation)
			assert.Len(t, sp.Load().APS, test.Remaining)
		})
	}
}

go/lib/pathmgr/pathmgr_test.go, line 502 at r2 (raw file):

	var ss []string
	for _, v := range aps {
		ss = append(ss, fmt.Sprintf("%v", v.Entry.Path.Interfaces))

If you do: ss = append(ss, strings.Trim(fmt.Sprintf("%v", v.Entry.Path.Interfaces), "[]"))
you can replace a lot of calls with:
assert.ElementsMatch(t, getPathStrings(aps), paths)

e.g. in TestQuery the two assert.Contains calls

go/lib/pathmgr/pathmgr_test.go, line 507 at r2 (raw file):

}

func apsCheckPaths(desc string, aps spathmeta.AppPathSet, expValues ...string) {

remove this

karampok

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @karampok)

go/lib/pathmgr/pathmgr_test.go, line 58 at r1 (raw file):

Previously, Oncilla wrote…

use buildSDAnswer instead:
buildSDAnswer("1-ff00:0:133#1019 1-ff00:0:132#1910 1-ff00:0:132#1916 1-ff00:0:131#1619")

Done.

go/lib/pathmgr/pathmgr_test.go, line 61 at r1 (raw file):

Previously, Oncilla wrote…

replace this by assert.Len(t, 1, len(aps))

Done.

go/lib/pathmgr/pathmgr_test.go, line 62 at r1 (raw file):

Previously, Oncilla wrote…

replace by assert.Contains(t, getPathStrings(aps), "... ")

Done.

go/lib/pathmgr/pathmgr_test.go, line 67 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.

go/lib/pathmgr/util_test.go, line 34 at r1 (raw file):

Previously, Oncilla wrote…

The same can be achieved with buildSDAnswer

Done.

karampok

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @karampok and @oncilla)

go/lib/pathmgr/pathmgr_test.go, line 53 at r2 (raw file):

Previously, Oncilla wrote…

Ah, I mislead you.

Judging from the sub tests name, they should actually not be sub tests and simply run sequentially.
i.e. just drop the t.Run -> tests should still pass
keep the sub test name as comment

Done.

go/lib/pathmgr/pathmgr_test.go, line 112 at r2 (raw file):

Previously, Oncilla wrote…

replace with require.NoError(t, err) here and everywhere else.

Done.

go/lib/pathmgr/pathmgr_test.go, line 163 at r2 (raw file):

Previously, Oncilla wrote…

no need for .String Sprintf will call the String method internally.

Done.

go/lib/pathmgr/pathmgr_test.go, line 310 at r2 (raw file):

Previously, Oncilla wrote…

s/a 5 sec time period/5 time units/

Done.

go/lib/pathmgr/pathmgr_test.go, line 325 at r2 (raw file):

Previously, Oncilla wrote…

What about:

func TestRevoke(t *testing.T) {
	t.Log("Given a path manager and a watch that")

	src := xtest.MustParseIA("1-ff00:0:111")
	dst := xtest.MustParseIA("1-ff00:0:110")
	paths := []string{
		"1-ff00:0:111#105 1-ff00:0:130#1002 1-ff00:0:130#1004 1-ff00:0:110#2",
		"1-ff00:0:111#104 1-ff00:0:120#5 1-ff00:0:120#6 1-ff00:0:110#1",
	}
	tests := map[string]struct {
		Paths         []string
		RevReply      *sciond.RevReply
		RevReplyError error
		Revocation    *path_mgmt.SignedRevInfo
		Remaining     int
	}{
		"retrieves one path revoking an IFID that matches the path": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevValid},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  0,
		},

		"retrieves one path revoking an IFID that does not match the path": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevValid},
			Revocation: newTestRev(t, "2-ff00:0:1#1"),
			Remaining:  1,
		},
		"tries to revoke an IFID, but SCIOND encounters an error": {
			Paths:         paths[:1],
			RevReplyError: errors.New("some error"),
			Revocation:    newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:     1,
		},
		"tries to revoke an IFID, but the revocation is invalid": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevInvalid},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  1,
		},
		"tries to revoke an IFID, but the revocation is stale": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevStale},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  1,
		},
		"tries to revoke an IFID, but the revocation is unknown": {
			Paths:      paths[:1],
			RevReply:   &sciond.RevReply{Result: sciond.RevUnknown},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  0,
		},
		"retrieves two paths and revokes an IFID that matches one path": {
			Paths:      paths,
			RevReply:   &sciond.RevReply{Result: sciond.RevValid},
			Revocation: newTestRev(t, "1-ff00:0:130#1002"),
			Remaining:  1,
		},
	}
	for name, test := range tests {
		t.Run(name, func(t *testing.T) {
			ctrl := gomock.NewController(t)
			defer ctrl.Finish()
			sd := mock_sciond.NewMockConnector(ctrl)
			pr := New(sd, Timers{})

			sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).Return(
				buildSDAnswer(test.Paths...), nil,
			)
			sd.EXPECT().Paths(gomock.Any(), dst, src, gomock.Any(), gomock.Any()).Return(
				buildSDAnswer(), nil,
			).AnyTimes()
			sp, err := pr.Watch(context.Background(), src, dst)
			require.NoError(t, err)
			sd.EXPECT().RevNotification(gomock.Any(), gomock.Any()).Return(
				test.RevReply, test.RevReplyError,
			)
			pr.Revoke(context.Background(), test.Revocation)
			assert.Len(t, sp.Load().APS, test.Remaining)
		})
	}
}

Done.

go/lib/pathmgr/pathmgr_test.go, line 502 at r2 (raw file):

Previously, Oncilla wrote…

If you do: ss = append(ss, strings.Trim(fmt.Sprintf("%v", v.Entry.Path.Interfaces), "[]"))
you can replace a lot of calls with:
assert.ElementsMatch(t, getPathStrings(aps), paths)

e.g. in TestQuery the two assert.Contains calls

Done.

go/lib/pathmgr/pathmgr_test.go, line 507 at r2 (raw file):

Previously, Oncilla wrote…

remove this

Done.

karampok

Reviewable status: 2 of 4 files reviewed, 8 unresolved discussions (waiting on @oncilla)

go/lib/pathmgr/pathmgr_test.go, line 92 at r2 (raw file):

Previously, Oncilla wrote…

As an idea, the test could also be written as below.
But this is fine, leave it as is if you want

func TestQueryFilterA(t *testing.T) {
	t.Log("Query with policy filter")

	srcIA := xtest.MustParseIA("1-ff00:0:133")
	dstIA := xtest.MustParseIA("1-ff00:0:131")
	paths := []string{
		"1-ff00:0:133#1019 1-ff00:0:132#1910 " + "1-ff00:0:132#1916 1-ff00:0:131#1619",
	}
	tests := map[string]struct {
		Policy       func() *pathpol.Policy
		ShouldFilter bool
	}{
		"Hop does not exist in paths, default deny": {
			Policy: func() *pathpol.Policy {
				pp, err := pathpol.HopPredicateFromString("0-0#0")
				require.NoError(t, err)
				policy := &pathpol.Policy{ACL: &pathpol.ACL{Entries: []*pathpol.ACLEntry{
					{Action: pathpol.Allow, Rule: pp},
					denyEntry,
				}}}
				return policy
			},
		},
		"Hop does not exist paths, default allow": {
			Policy: func() *pathpol.Policy {
				pp, err := pathpol.HopPredicateFromString("1-ff00:0:134#1910")
				require.NoError(t, err)
				policy := &pathpol.Policy{ACL: &pathpol.ACL{Entries: []*pathpol.ACLEntry{
					{Action: pathpol.Allow, Rule: pp},
					allowEntry,
				}}}
				return policy
			},
		},
		"Hop exists in paths, default deny ": {
			Policy: func() *pathpol.Policy {
				pp, err := pathpol.HopPredicateFromString("1-ff00:0:132#1910")
				require.NoError(t, err)
				policy := &pathpol.Policy{ACL: &pathpol.ACL{Entries: []*pathpol.ACLEntry{
					{Action: pathpol.Deny, Rule: pp},
					denyEntry,
				}}}
				return policy
			},
			ShouldFilter: true,
		},
	}
	for name, test := range tests {
		t.Run(name, func(t *testing.T) {
			ctrl := gomock.NewController(t)
			defer ctrl.Finish()
			sd := mock_sciond.NewMockConnector(ctrl)
			pm := New(sd, Timers{})
			sd.EXPECT().Paths(gomock.Any(), dstIA, srcIA, gomock.Any(), gomock.Any()).Return(
				buildSDAnswer(paths...), nil,
			).AnyTimes()
			aps := pm.QueryFilter(context.Background(), srcIA, dstIA, test.Policy())
			if test.ShouldFilter {
				assert.Len(t, aps, 0, "no path should remain")
			} else {
				assert.Len(t, aps, 1, "only one path should remain")
				assert.ElementsMatch(t, getPathStrings(aps), paths)
			}
		})
	}
}

Done. Credits to you that. I Just place the ctrl on the top

oncilla

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

Replace the manual generated file mock.go to use the autogenerated mock_sciond This fixes the scionproto#2377 issue

karampok force-pushed the pub-replace-mock branch from 44b9010 to 2a4d52f Compare August 20, 2019 12:57

oncilla self-requested a review August 20, 2019 13:23

oncilla requested changes Aug 20, 2019

View reviewed changes

karampok force-pushed the pub-replace-mock branch 5 times, most recently from 2f3dfef to 0141e03 Compare August 21, 2019 14:27

oncilla requested changes Aug 21, 2019

View reviewed changes

karampok commented Aug 21, 2019

View reviewed changes

karampok commented Aug 22, 2019

View reviewed changes

karampok force-pushed the pub-replace-mock branch from 0141e03 to cdcff1d Compare August 22, 2019 09:34

karampok commented Aug 22, 2019

View reviewed changes

oncilla approved these changes Aug 22, 2019

View reviewed changes

karampok force-pushed the pub-replace-mock branch from cdcff1d to 44a24e8 Compare August 22, 2019 12:39

karampok added 2 commits August 22, 2019 14:57

Remove SCIOND API mock

0aa5387

Replace the manual generated file mock.go to use the autogenerated mock_sciond This fixes the scionproto#2377 issue

Remove goconvey and refactor pathmgr tests

ada9d2e

karampok force-pushed the pub-replace-mock branch from 44a24e8 to ada9d2e Compare August 22, 2019 12:58

karampok merged commit 2291d26 into scionproto:master Aug 22, 2019

karampok deleted the pub-replace-mock branch August 23, 2019 09:17

scrye mentioned this pull request Aug 30, 2019

Remove SCIOND API mock #2377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SCIOND API mock #3014

Remove SCIOND API mock #3014

karampok commented Aug 20, 2019 •

edited by kormat

Loading

oncilla left a comment

oncilla left a comment

karampok left a comment

karampok left a comment

karampok left a comment

oncilla left a comment

Remove SCIOND API mock #3014

Remove SCIOND API mock #3014

Conversation

karampok commented Aug 20, 2019 • edited by kormat Loading

oncilla left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

karampok left a comment

Choose a reason for hiding this comment

karampok left a comment

Choose a reason for hiding this comment

karampok left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

karampok commented Aug 20, 2019 •

edited by kormat

Loading