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

Fix ipv6 parsing in client #4632

Merged
merged 1 commit into from
Nov 2, 2015
Merged

Fix ipv6 parsing in client #4632

merged 1 commit into from
Nov 2, 2015

Conversation

miguelxpn
Copy link
Contributor

Old implementation of function ParseConnectionString would parse ipv6 incorrectly

@otoolep
Copy link
Contributor

otoolep commented Oct 31, 2015

Thanks for this.

What would be great is to see this code lifted out as a distinct function
and then some unit tests added.

On Friday, October 30, 2015, Miguel Xavier Penha Neto <
notifications@github.com> wrote:

Old implementation of function ParseConnectionString would parse ipv6

incorrectly

You can view, comment on, or merge this pull request online at:

#4632
Commit Summary

  • Fix ipv6 parsing in client

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#4632.

@otoolep
Copy link
Contributor

otoolep commented Oct 31, 2015

Scrap that. It's already a function. What I mean is can you add a unit test showing what would fail to parse correctly previously?

@miguelxpn
Copy link
Contributor Author

I added a test with ipv6 that fails with the previous implementation and works with the new one. No idea why the other test failed though, did I do something wrong when adding the new test?

@miguelxpn
Copy link
Contributor Author

Hmmm, weird, sent it again and now everything passed, seems there's an intermittent problem that's caused some tests to fail in the meta package.

@otoolep
Copy link
Contributor

otoolep commented Nov 1, 2015

Yeah, known issue with the tests in that package.

On Saturday, October 31, 2015, Miguel Xavier Penha Neto <
notifications@github.com> wrote:

Hmmm, weird, sent it again and now everything passed, seems there's an
intermittent problem that's caused some tests to fail in the meta package.


Reply to this email directly or view it on GitHub
#4632 (comment).

@@ -547,3 +547,14 @@ func TestClient_NoTimeout(t *testing.T) {
t.Fatalf("unexpected error. expected %v, actual %v", nil, err)
}
}

func TestClient_ParseConnectionStringi_Ipv6(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a rogue 'i' in the test name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, Go style dictates that this test should be named TestClient_ParseConnectionString_IPv6.

@otoolep
Copy link
Contributor

otoolep commented Nov 1, 2015

@corylanou

@corylanou
Copy link
Contributor

+1

Old implementation of function ParseConnectionString would parse ipv6 incorrectly

Added a unit test for this
@miguelxpn
Copy link
Contributor Author

@otoolep there was indeed a rogue 'i' in there! Fixed the test name, thanks for the feedback!

@otoolep
Copy link
Contributor

otoolep commented Nov 2, 2015

otoolep added a commit that referenced this pull request Nov 2, 2015
Fix ipv6 parsing in client
@otoolep otoolep merged commit 0fc8d4f into influxdata:master Nov 2, 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.

None yet

3 participants