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

Move all HTTP code from Server to HTTPTransport, add Transport interface #8

Merged
merged 10 commits into from
Sep 24, 2018

Conversation

jfbus
Copy link
Contributor

@jfbus jfbus commented Aug 24, 2018

The goal is to enable support for alternate transports

@coveralls
Copy link

coveralls commented Aug 24, 2018

Pull Request Test Coverage Report for Build 56

  • 113 of 155 (72.9%) changed or added relevant lines in 10 files are covered.
  • 4 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+14.5%) to 77.027%

Changes Missing Coverage Covered Lines Changed/Added Lines %
endpoint.go 7 9 77.78%
options.go 0 3 0.0%
log.go 9 12 75.0%
health.go 1 7 14.29%
server.go 22 31 70.97%
http.go 64 83 77.11%
Files with Coverage Reduction New Missed Lines %
options.go 1 0.0%
log.go 1 64.52%
health.go 2 33.33%
Totals Coverage Status
Change from base Build 35: 14.5%
Covered Lines: 285
Relevant Lines: 370

💛 - Coveralls

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
@@ -64,22 +51,22 @@ func LogEndpoint(fields ...LogOption) endpoint.Middleware {
return func(e endpoint.Endpoint) endpoint.Endpoint {
return func(ctx context.Context, request interface{}) (response interface{}, err error) {
if opts[LogRequest] {
LogMessage(ctx, fmt.Sprintf("request: %+v", request))
_ = LogMessage(ctx, fmt.Sprintf("request: %+v", request))
Copy link

@dolanor dolanor Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a LogMessage returns an error?
In what case can it be produced?
Won't it allow us to see we have a problem with our logging system and thus, our capacity of detecting an error on the system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you cannot log the error return by your logger... I don't see what you can do.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about falling back to the classic "log" in case of an error so at least you can figure out that there is a problem.

http.go Outdated Show resolved Hide resolved
Copy link

@dolanor dolanor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running go test from the current version gave me 2 different errors:

$ go test -count 1 ./...
ok  	github.com/objenious/kitty	0.055s
ok  	github.com/objenious/kitty/backoff	1.289s
ok  	github.com/objenious/kitty/circuitbreaker	0.003s
--- FAIL: TestRouter (0.00s)
	mux_test.go:70: the not found handler was not called
FAIL
FAIL	github.com/objenious/kitty/gorilla	0.005s
?   	github.com/objenious/kitty/test	[no test files]

and

$ go test -count 1 ./...
--- FAIL: TestServer (1.00s)
	server_test.go:72: receive a 200 status instead of 404
	server_test.go:84: A decoding error should return a BadRequest status, not 404
	server_test.go:92: Server.Run has not stopped after 1sec
FAIL
FAIL	github.com/objenious/kitty	1.005s
ok  	github.com/objenious/kitty/backoff	1.211s
ok  	github.com/objenious/kitty/circuitbreaker	0.003s
ok  	github.com/objenious/kitty/gorilla	0.054s
?   	github.com/objenious/kitty/test	[no test files]

Using the race detector I got:

$ go test -race -count 1 ./...
==================
WARNING: DATA RACE
Read at 0x00c4200a4645 by goroutine 26:
  github.com/objenious/kitty.TestServer()
      /home/dolanor/go/jenious/src/github.com/objenious/kitty/server_test.go:98 +0xe04
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d

Previous write at 0x00c4200a4645 by goroutine 31:
  github.com/objenious/kitty.TestServer.func1()
      /home/dolanor/go/jenious/src/github.com/objenious/kitty/server_test.go:22 +0x46
  github.com/objenious/kitty.(*Server).Run.func2()
      /home/dolanor/go/jenious/src/github.com/objenious/kitty/server.go:65 +0xde

Goroutine 26 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:824 +0x564
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1063 +0xa4
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1061 +0x4e1
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:978 +0x2cd
  main.main()
      _testmain.go:50 +0x22a

Goroutine 31 (finished) created at:
  github.com/objenious/kitty.(*Server).Run()
      /home/dolanor/go/jenious/src/github.com/objenious/kitty/server.go:62 +0x39d
  github.com/objenious/kitty.TestServer.func2()
      /home/dolanor/go/jenious/src/github.com/objenious/kitty/server_test.go:25 +0x4c
==================
--- FAIL: TestServer (1.01s)
	server_test.go:72: receive a 200 status instead of 404
	server_test.go:84: A decoding error should return a BadRequest status, not 404
	server_test.go:92: Server.Run has not stopped after 1sec
	testing.go:730: race detected during execution of test
FAIL
FAIL	github.com/objenious/kitty	1.019s
ok  	github.com/objenious/kitty/backoff	2.061s
ok  	github.com/objenious/kitty/circuitbreaker	1.010s
ok  	github.com/objenious/kitty/gorilla	1.064s
?   	github.com/objenious/kitty/test	[no test files]

@jfbus jfbus merged commit 6f50669 into master Sep 24, 2018
@jfbus jfbus deleted the refacto/http_transport branch September 24, 2018 19:33
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.

None yet

4 participants