Create CT verification client #259
Conversation
gdbelvin
changed the title
|
|
||
| "golang.org/x/net/context" | ||
|
|
||
| ct "github.com/google/certificate-transparency/go" |
There was a problem hiding this comment.
How are you organizing your imports? It seems to be sort of arbitrary between the files in this PR.
There was a problem hiding this comment.
The scheme I intended to follow:
- Go packages
- Local repository packages
- 3rd Party packages
- Protobuf packages
|
Cleaned up tests. @branlwyd PTAL and see if you're happy :-) |
| if err != nil { | ||
| t.Fatalf("NewLog(): %v", err) | ||
| } | ||
| for i := range tests { |
There was a problem hiding this comment.
I would like to do this, however, I need tc to be a variable outside the for loop so the http server can access it. Any other suggestions? I could create http.NewServer inside the test, but this would involve opening and closing many sockets - perhaps increasing test flakiness and latency.
There was a problem hiding this comment.
nit: once the above comment is addressed, if you decided to create a new httptest.Server for each test case this can be written as for i, tc := range tests {
| }, | ||
| } | ||
| var tc Test | ||
| hs := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
How expensive is it to create these HTTP test servers?
It's kind of hacky that tc is visible outside of the test-case loop, and AFAICT this is only so hs's handler func can examine the test case values. This also means that the test cases are not completely isolated from one another--if one test case somehow put the the httptest.Server in a broken state, later test cases could observe this brokenness.
I recommend creating a new httptest.Server for each test case, unless it is very expensive to do so.
(it looks like a few tests have this issue)
|
Added a couple more comments. LGTM once these are addressed. |
| t.Fatalf("Incorrect URL path: %s", r.URL.Path) | ||
| } | ||
| })) | ||
| defer hs.Close() |
There was a problem hiding this comment.
This deferred hs.Close() won't run until the entire test terminates. There are only a few test cases per test, but I suggest either:
- Manually calling close at the end of each test case.
- Wrapping the per-test-case code in an anonymous function call so that the defer is effectively scoped to the test case loop iteration.
|
Moves CT verification from the
clientto it's ownLogobject.This closes #227 and closes #171.