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

Add Graphite Integration Test #1758

Merged
merged 8 commits into from
Mar 11, 2015
Merged

Add Graphite Integration Test #1758

merged 8 commits into from
Mar 11, 2015

Conversation

corylanou
Copy link
Contributor

This PR is to address issue ##1735.

@otoolep
Copy link
Contributor

otoolep commented Feb 26, 2015

Looking good @corylanou -- badly needed.

@@ -50,9 +50,14 @@ func Run(config *Config, join, version string, logWriter *os.File) (*messaging.B
// We want to make sure we are spun up before we exit this function, so we manually listen and serve
listener, err := net.Listen("tcp", config.BrokerAddr())
if err != nil {
log.Fatal(err)
log.Fatalf("failed to listen for broker on tcp: %s ", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion: include the value of config.BrokerAddr() in the log message. It might be better phrased as something like "Broker failed to listen on %s: %s" where the first variable is the address, the second is the error. Brokers only listen over TCP, so mentioning TCP isn't strictly necessary.

@corylanou corylanou force-pushed the start-graphite branch 2 times, most recently from 10c18ed to 8a9a635 Compare March 6, 2015 16:42
@toddboom
Copy link
Contributor

toddboom commented Mar 6, 2015

@corylanou is this guy ready to go?

@otoolep
Copy link
Contributor

otoolep commented Mar 6, 2015

This test is very brittle, just so you know.

@corylanou
Copy link
Contributor Author

It's why I haven't merged it yet :-) It's only brittle because of a bug that is in the broker that Ben is currently fixing. I'm going to wait until he has that fixed and then update all the tests to re-enable index/wait endpoints. This will significantly speed up the integration tests, which are now taking almost a minute to run.

@toddboom
Copy link
Contributor

toddboom commented Mar 6, 2015

(continues to wait patiently)

@corylanou
Copy link
Contributor Author

Fixed at brittle test methodology. We now have a wait that will poll for expected results, or times out.

Ready for review.

@@ -167,9 +173,6 @@ func write(t *testing.T, node *Node, data string) {
body, _ := ioutil.ReadAll(resp.Body)
t.Fatalf("Write to database failed. Unexpected status code. expected: %d, actual %d, %s", http.StatusOK, resp.StatusCode, string(body))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Legend!

@otoolep
Copy link
Contributor

otoolep commented Mar 11, 2015

You are adding a file called influxd.exe. What is going on there?

} else if atomic.LoadInt32(&timedOut) == 1 {
return fmt.Sprintf("timed out before expected result was found: got: %s", got), false
} else {
time.Sleep(100 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see no reason why this can't be 10 milliseconds.

@otoolep
Copy link
Contributor

otoolep commented Mar 11, 2015

+1

corylanou added a commit that referenced this pull request Mar 11, 2015
@corylanou corylanou merged commit 41607c2 into master Mar 11, 2015
@corylanou corylanou removed the review label Mar 11, 2015
@corylanou corylanou deleted the start-graphite branch March 11, 2015 22:13
corylanou added a commit that referenced this pull request Mar 11, 2015
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.

3 participants