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

Remove Convey from layers package #3051

Merged
merged 3 commits into from
Aug 28, 2019

Conversation

chaehni
Copy link

@chaehni chaehni commented Aug 27, 2019


This change is Reviewable

@chaehni chaehni changed the title HP: add hidden path segment extension Remove Convey from layers package Aug 27, 2019
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 5 files at r1, 2 of 3 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @chaehni)


go/lib/layers/extensions.go, line 63 at r1 (raw file):

func (o *ExtnOHP) DecodeFromLayer(extension *Extension) error {
	if len(extension.Data) != common.ExtnFirstLineLen {

good catch


go/lib/layers/extensions_layer_test.go, line 77 at r2 (raw file):

			var extn Extension
			err := extn.DecodeFromBytes(tc.Data, gopacket.NilDecodeFeedback)
			if tc.ExpectedError {

since we do this already:
change ExpectedError bool to: ErrorAssertion assert.ErrorAssertionFunc and ExpectedError: true to ErrorAssertion: assert.Errorm ExpectedError: false to ErrorAssertion: assert.NoError.
Then the code here is simply:

tc.ErrorAssertion(t, err)
assert.Equal(t, tc.ExpectedExtension, extn, "extension must match")

go/lib/layers/extensions_layer_test.go, line 97 at r2 (raw file):

		ExpectedLength uint8
	}
	testCases := []*TestCase{

bonus points for changing to

map[string]Testcase{
 "description": {....}

also bonus points for:

  • testCases -> tests rename
  • tc -> test rename.

so that we have for name, test := range tests {...
but feel free to skip that.


go/lib/layers/extensions_layer_test.go, line 167 at r2 (raw file):

			b := gopacket.NewSerializeBuffer()
			err := tc.Extension.SerializeTo(b, tc.SerializeOptions)
			if tc.ExpectedError {

Ditto error assertion


go/lib/layers/extensions_test.go, line 84 at r2 (raw file):

			var extn ExtnSCMP
			err := extn.DecodeFromLayer(tc.Extension)
			if tc.ExpectedError {

ditto


go/lib/layers/extensions_test.go, line 115 at r2 (raw file):

			var extn ExtnUnknown
			err := extn.DecodeFromLayer(tc.Extension)
			if tc.ExpectedError {

ditto

@oncilla
Copy link
Contributor

oncilla commented Aug 27, 2019


go/lib/layers/extensions_layer_test.go, line 77 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

since we do this already:
change ExpectedError bool to: ErrorAssertion assert.ErrorAssertionFunc and ExpectedError: true to ErrorAssertion: assert.Errorm ExpectedError: false to ErrorAssertion: assert.NoError.
Then the code here is simply:

tc.ErrorAssertion(t, err)
assert.Equal(t, tc.ExpectedExtension, extn, "extension must match")

I would use require.ErrorAssertionFunc, no point in continuing, if the error is not as expected.

Copy link
Author

@chaehni chaehni 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: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)


go/lib/layers/extensions_layer_test.go, line 77 at r2 (raw file):

Previously, Oncilla wrote…

I would use require.ErrorAssertionFunc, no point in continuing, if the error is not as expected.

Done.


go/lib/layers/extensions_layer_test.go, line 167 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Ditto error assertion

Done.


go/lib/layers/extensions_test.go, line 84 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ditto

Done.


go/lib/layers/extensions_test.go, line 115 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ditto

Done.

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 r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @chaehni)


go/lib/layers/extensions_layer_test.go, line 28 at r3 (raw file):

func TestExtensionDecodeFromBytes(t *testing.T) {
	type TestCase struct {
		Description       string

This field is no longer needed and can be removed.


go/lib/layers/extensions_layer_test.go, line 82 at r3 (raw file):

func TestExtensionSerializeTo(t *testing.T) {
	type TestCase struct {
		Description      string

ditto


go/lib/layers/extensions_test.go, line 27 at r3 (raw file):

func TestExtnOHPDecodeFromLayer(t *testing.T) {
	type TestCase struct {
		Description    string

ditto & for the rest of the struct in this file

Claude Hähni added 3 commits August 28, 2019 10:18
Copy link
Author

@chaehni chaehni 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: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)


go/lib/layers/extensions_layer_test.go, line 28 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

This field is no longer needed and can be removed.

Done.


go/lib/layers/extensions_layer_test.go, line 82 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ditto

Done.


go/lib/layers/extensions_test.go, line 27 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ditto & for the rest of the struct in this file

Done.

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.

:lgtm: thanks

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

@lukedirtwalker lukedirtwalker merged commit a721b31 into scionproto:master Aug 28, 2019
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.

3 participants