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

[ADDED] ReconnectBufSize() Option function #340

Merged
merged 4 commits into from
Jan 25, 2018
Merged

Conversation

ripienaar
Copy link
Contributor

This creates a function one can use to set the ReconnectBufSize option,
it's similar in design to the other functions already here

Also makes small fixes to some doc strings that I noticed

This creates a function one can use to set the ReconnectBufSize option,
it's similar in design to the other functions already here

Also makes small fixes to some doc strings that I noticed
@coveralls
Copy link

coveralls commented Jan 25, 2018

Coverage Status

Coverage increased (+0.01%) to 94.649% when pulling f5676e9 on ripienaar:339 into 78ec4b9 on nats-io:master.

@ripienaar
Copy link
Contributor Author

test failure does not appear to be my doing :)

@kozlovic
Copy link
Member

I have recycled the test. Would you mind adding a test so that code coverage shows that this function was invoked? It could be as simple as creating a connection with that option and then checking that the internal reconnect buffer size matches what you have set.

@ripienaar
Copy link
Contributor Author

OK - I did check, none of these others are tested, but sure.

@kozlovic
Copy link
Member

From coverall, it seems that they are since they show in green. Keep in mind that there are tests in test directory and in nats_test.go

@ripienaar
Copy link
Contributor Author

indeed, I saw there's a nats_test.go and figured if its tested its in there, missed the test dir :)

@kozlovic
Copy link
Member

Thanks, but that is really to make coverall happy. Could you instead look at test TestReconnectBufSize and make it a table test where you run the same content but once with option set, the way it is right now, and once with the new option you added?
If you can't that's fine, we can add it after the PR is merged.

@ripienaar
Copy link
Contributor Author

erm, I guess, though not really experienced with go's built in test framework, will have to wait for another day as that's all i had time for today

@derekcollison
Copy link
Member

Thanks for the submission, this would be helpful. We do require tests though.

@ripienaar
Copy link
Contributor Author

yup, will look at test tomorrow

@@ -700,7 +700,9 @@ func TestCallbacksOrder(t *testing.T) {
nats.ClosedHandler(cch),
nats.ErrorHandler(ech),
nats.ReconnectWait(50*time.Millisecond),
nats.DontRandomize())
nats.DontRandomize(),
nats.ReconnectBufSize(10*1024))
Copy link
Member

Choose a reason for hiding this comment

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

Since you added a specific test, you may want to revert this change.

@ripienaar
Copy link
Contributor Author

@kozlovic looks like reworking that test into a table test is quite an undertaking - but as per your earlier suggestion the thing to test is if the option gets set or not, since that other test makes sure that the rest of the code does with whatever was set.

So I reckon this is sufficient to test the functionality of the Option.

There appears to be some weirdness, maybe something in my environments but these tests panic for me on timeout - even before my changes. But seems ok on travis :)

@kozlovic
Copy link
Member

@ripienaar You may have a server running outside of the test suite...

@kozlovic
Copy link
Member

LGTM

@ripienaar
Copy link
Contributor Author

ok, let me know if there's anything else, but that'll be for tomorrow, thanks!

@derekcollison
Copy link
Member

Thanks! @kozlovic will merge.

@kozlovic kozlovic changed the title adds a ReconnectBufSize Option function [ADDED] ReconnectBufSize() Option function Jan 25, 2018
@kozlovic kozlovic merged commit c7e16c2 into nats-io:master Jan 25, 2018
@kozlovic
Copy link
Member

Thanks for the contribution!

@ripienaar ripienaar deleted the 339 branch June 29, 2021 08:46
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.

4 participants