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

messenger: use addr.AppAddr less in tests #3420

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

karampok
Copy link
Contributor

@karampok karampok commented Nov 22, 2019

As part of the remote add.AppAddr effort.


This change is Reviewable

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, 6 unresolved discussions (waiting on @karampok)

a discussion (no related file):
title: messenger: Use addr.AppAddr less and remove Convey tests



go/lib/addr/util.go, line 36 at r1 (raw file):

	}
}
func NewUDPAppAddr(u *net.UDPAddr) *AppAddr {

I think AppAddrFromUDP does everything you need ;)


go/lib/infra/messenger/addr_test.go, line 42 at r1 (raw file):

		input net.Addr
		want  *snet.Addr
		af    assert.ErrorAssertionFunc

asssertErr ? I think af refers to the type I don't care about the type. reading below tc.af does not tell me anything. tc.assertErr is a bit more clear.


go/lib/infra/messenger/addr_test.go, line 227 at r1 (raw file):

func TestResolveIfSVC(t *testing.T) {
	testCases := []struct {
		dscr                  string

I personally don't like those short forms. why not just description I can immediately see what is meant. If I see dscr I don't immediately know what it means. And all that only to save a few characters?


go/lib/infra/messenger/addr_test.go, line 234 at r1 (raw file):

		want                  *addr.AppAddr
		wantQUICRedirect      bool
		af                    assert.ErrorAssertionFunc

assertErr


go/lib/infra/messenger/addr_test.go, line 343 at r1 (raw file):

		Reply *svc.Reply
		want  *net.UDPAddr
		af    assert.ErrorAssertionFunc

dittoassertErr

@karampok karampok force-pushed the pub-remove-appaddr-messenger branch from 788df2e to 500e276 Compare November 22, 2019 10:08
@karampok karampok changed the title messenger: less add.AppAddr and remove convey messenger: use addr.AppAddr less in tests Nov 22, 2019
Copy link
Contributor Author

@karampok karampok 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: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

title: messenger: Use addr.AppAddr less and remove Convey tests

Done.



go/lib/addr/util.go, line 36 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think AppAddrFromUDP does everything you need ;)

I will prefer to keep that because the later refactor will be more straightforward
(when NewUDP -> X, when NewSVC -?)

I brought the functions inside the test to isolate them.


go/lib/infra/messenger/addr_test.go, line 42 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

asssertErr ? I think af refers to the type I don't care about the type. reading below tc.af does not tell me anything. tc.assertErr is a bit more clear.

Done.


go/lib/infra/messenger/addr_test.go, line 227 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I personally don't like those short forms. why not just description I can immediately see what is meant. If I see dscr I don't immediately know what it means. And all that only to save a few characters?

Transformed to map.


go/lib/infra/messenger/addr_test.go, line 234 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

assertErr

Done.


go/lib/infra/messenger/addr_test.go, line 343 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

dittoassertErr

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:

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

* remove convey
* separate test package
* hide creation of addr.AppAddr for type UDP
under a function to allow easier later refactor
* add a basic unit-test for uncovered part

Contributes to `add.AppAddr` effort.
@karampok karampok force-pushed the pub-remove-appaddr-messenger branch from 500e276 to f845943 Compare November 22, 2019 10:28
@karampok karampok merged commit 5b718b4 into scionproto:master Nov 22, 2019
@karampok karampok deleted the pub-remove-appaddr-messenger branch November 22, 2019 12:12
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.

2 participants