Add testing to the proxy

[facchettos]

Which issue this PR addresses:

This PR adds some basic testing to the proxy.

What this PR does / why we need it:

This pr refactors the http server of the proxy in order to make it more testable. Tests are in proxy_test.go

Test plan for issue:

This adds tests which were not there before.

Is there any documentation that needs to be updated for this PR?

No

[karanmagdani]

Thanks for working on this @facchettos. LGTM! I just have one question for my understanding

[s-amann]

lgtm

[SudoBrendan]

LGTM! Only one nitpick you can choose to fix or not.

Great to see so many PRs so early, great job Jeremy!

[AldoFusterTurpin]

Just a question about the use of []struct instead of map[string]struct in proxy_test.go.

[AldoFusterTurpin]

Just a quick question.
Why instead of using a []struct, you use a map[string]struct where the key of the map is the test name and the value of the map is the struct you already have. This provides some benefits like:

• ensuring unique test case names (as the keys of a map are unique)
• Map iteration order is undefined. This means each time we run go test, our tests are going to be potentially run in a different order. This is super useful for spotting conditions where tests pass when run in statement order, but not otherwise. I don't think this is happening here but is a good practice to adopt.

More info in Dave Cheney blog, "Give your test cases names" section (or just look for "We can dry this up, even more, using a map literal syntax:").

This is just a comment, I would not reject this PR for that reason but don't feel enough confident to approve it either. 🙂

[facchettos]

I actually followed a recommendation from Mikalai from another PR. But this is a pattern that we have in most (if not all) other test files too.

The test cases will have a #0X appended to them if they have the same name in the struct. So it would probably be something we would lose with the map because the test would be overwritten, and we'd have to manually add the numbers to each of those. (now I actually don't know if having multiple subtests with the same name is something we want to have, so I'd wait for someone with more experience to weigh in)

[bennerv]

More info in Dave Cheney blog, "Give your test cases names" section (or just look for "We can dry this up, even more, using a map literal syntax:").

This is interesting. We shouldn't have to nest test cases as your test cases would evolve into something like the below. It shouldn't require an #0X appended as long as the names of the tests are different.

	tests := map[string]struct {
		method     string
		subnet     string
		hostname   string
		wantStatus int
		wantErr    bool
	}{
               "get https same subnet": {
			method:     http.MethodGet,
			subnet:     "127.0.0.1/24",
			hostname:   "https://127.0.0.2:123",
			wantStatus: http.StatusMethodNotAllowed,
			wantErr:    true,
		}, 
                "test-2": {
			method:     http.MethodGet,
			subnet:     "127.0.0.1/24",
			hostname:   "https://127.0.0.2:123",
			wantStatus: http.StatusMethodNotAllowed,
			wantErr:    true,
		}, 
		...
	}

That being said, I'm fine conforming to our current standard. Definitely something I would approve if it was changed going forward as well though. It is a small addition of DRY

[karanmagdani]

LGTM

[bennerv]

Great work! Couple nits, grammar, and question. otherwise LGTM

[bennerv]

LGTM - can merge on green