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 two more tests for multiple and no api-endpoints #211

Merged
merged 3 commits into from Jul 2, 2014
Merged

Added two more tests for multiple and no api-endpoints #211

merged 3 commits into from Jul 2, 2014

Conversation

themue
Copy link
Contributor

@themue themue commented Jul 1, 2014

Added two more api-endpoint test cases for multiple endpoints (and using only the first one) and no endpoints (so automatically refresh and return the first endpoint then).


// modifyAddresses adds more endpoint addresses or removes all
// in case of nil.
func (s *EndpointSuite) modifyAddresses(c *gc.C, addresses []string) {
Copy link

Choose a reason for hiding this comment

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

I find such function confusing: adds more endpoint addresses or removes all in case of nil.
The two separate function would be more appropriate here.
But even if we stay the above function, I would do nothing if called modifyAddresses(c, []string{}),
and delete all addresses only if called modifyAddresses(c, nil).

Anyway, the current comment is invalid, because now all addresses are removed in case of []string{}, too.

Copy link
Member

Choose a reason for hiding this comment

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

Seems a fair point. What if we just simplify all of it to a simple "setAPIAddresses". And then we can call it with an empty list, or with a known list.
We just move the call to s.APIInfo() a bit earlier, and then do:
s.setAPIAddresses(c, []string{info.Addrs[0], "0.1.2.3...", "0.1.2.3..."})

@jameinel
Copy link
Member

jameinel commented Jul 2, 2014

I think you could go with a setAPIAddresses, and then it LGTM.

@themue
Copy link
Contributor Author

themue commented Jul 2, 2014

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Jul 2, 2014

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

jujubot added a commit that referenced this pull request Jul 2, 2014
Added two more tests for multiple and no api-endpoints

Added two more api-endpoint test cases for multiple endpoints (and using only the first one) and no endpoints (so automatically refresh and return the first endpoint then).
@jujubot jujubot merged commit 74d0788 into juju:master Jul 2, 2014
@themue themue deleted the enhance-api-endpoints-tests branch July 2, 2014 09:42
laszlokajtar pushed a commit to laszlokajtar/juju that referenced this pull request Oct 25, 2023
…torials

Implement filters on tutorials page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants