[WIP] Issue36 - Eureka integration #46
Conversation
…or eureka compat, and an in progress implementation
…oes not fix the issue. To follow up
discovery/eureka.go
Outdated
| } else if resp.StatusCode != 200 { | ||
| return []string{},errEurekaUnexpectedHttpResponseCode | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
I guessed you will return the string array with the IPs here instead of nil...
discovery/eureka_test.go
Outdated
| var eurekaTestPort int = 8080 | ||
| var eurekaTestUrl string = "http://127.0.0.1:" + strconv.Itoa(eurekaTestPort) + "/eureka" | ||
|
|
||
| func TestMain(m *testing.M) { |
There was a problem hiding this comment.
what do you mean with TestMain?
There was a problem hiding this comment.
discovery/eureka_test.go
Outdated
|
|
||
| func TestMain(m *testing.M) { | ||
| // uses a sensible default on windows (tcp/http) and linux/osx (socket) | ||
| pool, err := dockertest.NewPool("") |
There was a problem hiding this comment.
why are you using "dockertest" instead of "dockertest.v3"? has the dot no sense?
There was a problem hiding this comment.
discovery/eureka_test.go
Outdated
| log.Fatalf("Could not connect to docker: %s", err) | ||
| } | ||
| // pulls an image, creates a container based on it and runs it | ||
| resource, err := pool.RunWithOptions(&dockertest.RunOptions{ |
There was a problem hiding this comment.
this resource is the Eureka container running, isn't it? a more descriptive name could be great 😄
There was a problem hiding this comment.
👍 . I was following the examples to strictly 😸
| log.Fatalf("Could not connect to the docker resource: %s", err) | ||
| } | ||
|
|
||
| code := m.Run() |
There was a problem hiding this comment.
what is Run from Testing doing?
There was a problem hiding this comment.
See the previous reference about what TestMain actually does
discovery/eureka_test.go
Outdated
| func TestEurekaClientWrongEurekaContext(t *testing.T) { | ||
| _, err := NewEurekaClient(eurekaTestUrl + "badsuffix") | ||
| if err != errEurekaUnexpectedHttpResponseCode { | ||
| t.Fatal("Eureka should be reachable but, when asking a wrong URL, it should return a non 200 response code") |
There was a problem hiding this comment.
mmmm, reaching this point I suspect that the m.Run() in TestMain does is to start the Eureka server to run the other tests after that. am I right?
There was a problem hiding this comment.
Nops. TestMain does run Eureka (and kills it when tests finish), and in the middle, with m.Run , actually runs all your tests there. Its a mechanism allowing you to wrap all your tests and do things at the beginning and end (init only covers the first part, so that's why golang released this in... 1.5?)
There was a problem hiding this comment.
👍 seen in your comment above
| } | ||
|
|
||
| func TestEurekaClientEurekaDoesNotReply(t *testing.T) { | ||
| _, err := NewEurekaClient("http://192.0.2.1:9999/thisshouldtimeout") |
There was a problem hiding this comment.
"Addresses starting with "192.0.2.", "198.51.100.", or "203.0.113." are reserved for use in documentation and sample configurations. They should never be used in a live network configuration. No one has permission to use these addresses on the Internet."
I guess doing testing is no documentation...
There was a problem hiding this comment.
Can you suggest another IP range which traffic will be dropped for sure? In fact, these networks are referenced as "TEST NETWORKs" in the RFC, so that was the best option I found...
There was a problem hiding this comment.
you were right, don't change it.
discovery/eureka.go
Outdated
| httpclient := http.Client{Timeout: time.Second * eurekaClientTimeoutInSeconds} | ||
| resp, err := httpclient.Get(eurekaAppUrl) | ||
| req, err := http.NewRequest("GET", eurekaAppUrl, nil) |
There was a problem hiding this comment.
why did you change from httpclient.Get to http.NewRequest + httpclient.Do?
There was a problem hiding this comment.
Because I'm trying to add a request header, and the Get method does not allow me so
discovery/eureka_test.go
Outdated
| @@ -24,8 +24,10 @@ func TestMain(m *testing.M) { | |||
| resource, err := pool.RunWithOptions(&dockertest.RunOptions{ | |||
| Repository: "netflixoss/eureka", | |||
| Tag: "1.3.1", | |||
| PortBindings: map[dc.Port][]dc.PortBinding{ | |||
| dc.Port(strconv.Itoa(eurekaTestPort) + "/tcp"): {{HostIP: "", HostPort: strconv.Itoa(eurekaTestPort)}}, | |||
| //Repository: "containers.schibsted.io/spt-infrastructure/eureka-docker", | |||
There was a problem hiding this comment.
what is this comment for? own testing?
There was a problem hiding this comment.
WIP. Comparing the (old) public version in eureka with one that is newer, but not public...
I started to cover this:
#36
And following a tdd approach... tests are quite complete (but end to end), and main pending points are:
Feel free to start sharing feedback, @chadell