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

HP: Registration Handler tests #3123

Merged

Conversation

chaehni
Copy link

@chaehni chaehni commented Sep 6, 2019

adds tests for the HP registration handler


This change is Reviewable

@chaehni chaehni force-pushed the hidden_path_implementation branch from 85c8f70 to db4a991 Compare September 6, 2019 14:18
@lukedirtwalker lukedirtwalker self-requested a review September 6, 2019 14:37
@lukedirtwalker lukedirtwalker self-assigned this Sep 6, 2019
@lukedirtwalker lukedirtwalker added feature New feature or request c/hidden paths Hidden paths service labels Sep 6, 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 4 of 4 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chaehni)


go/hidden_path_srv/internal/registration/handler_test.go, line 153 at r1 (raw file):

				context.Background(), mocks.rw)
			handler := registration.NewSegRegHandler(
				registration.NullValidator,

Instead of always using a NullValidator you could also use a mock validator, then you could also test that failing validation triggers the correct action.
For that you would need to modify tools/gomocks, then run make mocks then you can use the mock here: mock_registration.NewMockValidator(ctrl)


go/hidden_path_srv/internal/registration/validator.go, line 104 at r1 (raw file):

// NullValidator validates all input
var NullValidator = nullValidator{}

If it's only used for testing then I would also define it in the test file. I don't think this ever makes sense in non-testing code.

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


go/hidden_path_srv/internal/registration/handler_test.go, line 153 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Instead of always using a NullValidator you could also use a mock validator, then you could also test that failing validation triggers the correct action.
For that you would need to modify tools/gomocks, then run make mocks then you can use the mock here: mock_registration.NewMockValidator(ctrl)

You're right. That makes more sense.


go/hidden_path_srv/internal/registration/validator.go, line 104 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

If it's only used for testing then I would also define it in the test file. I don't think this ever makes sense in non-testing code.

With the mock Validator its no longer needed. I have removed it.

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 6 of 6 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker lukedirtwalker merged commit b842fac into scionproto:master Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/hidden paths Hidden paths service feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants