Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Relation add and remove commands improvements. #6334
Conversation
jameinel
approved these changes
Sep 27, 2016
I have a couple comments but mostly it looks very good.
| - c.Assert(err, gc.ErrorMatches, t.err) | ||
| - } | ||
| +func (s *AddRelationSuite) TestAddRelationFail(c *gc.C) { | ||
| + msg := "fail add relation" |
jameinel
Sep 27, 2016
Owner
I guess the message doesn't matter too much but this doesn't read like proper English
anastasiamac
Sep 27, 2016
Member
Well, it is proper in a sense that it says that it's failed add/remove relation but I have re-phrased to be more obvious ;)
Let me know if it reads better and helps to understand the test.
| + return common.OperationBlockedError("TestRemoveRelationBlocked") | ||
| + } | ||
| + err := s.runRemoveRelation(c, "application1", "application2") | ||
| + coretesting.AssertOperationWasBlocked(c, err, ".*TestRemoveRelationBlocked.*") |
jameinel
Sep 27, 2016
Owner
I feel like this might be missing a check of whether the error is something that is actually helpful for the end user. Thoughts?
anastasiamac
Sep 27, 2016
Member
We do not return anything user-friendly to console. We log blocked operations. I wonder if it's right though... If it's not, I am tempted to address it in a separate PR as it will involve change in behavior of all blocked commands.
I have added a check to ensure that the log gets correct message with currently existing behavior.
| @@ -34,28 +44,26 @@ func (c *addRelationCommand) Info() *cmd.Info { | ||
| } | ||
| func (c *addRelationCommand) Init(args []string) error { | ||
| + // Must have only 2 arguments |
axw
Sep 28, 2016
•
Edited 1 time
-
axw
Sep 28, 2016
Member
-
Sep 28, 2016
this is evident from the line directly below, can we please do without this?
| if len(args) != 2 { | ||
| return errors.Errorf("a relation must involve two applications") | ||
| } | ||
| + // None of the arguments can be empty. | ||
| + if args[0] == "" || args[1] == "" { | ||
| + return errors.Errorf("a relation must involve two applications") |
axw
Sep 28, 2016
Member
This error message isn't great. It's not that a value wasn't specified, but it was empty. You really have to be trying to do that. I feel it would be more helpful to validate according to the names package, i.e. check that each arg matches
fmt.Sprintf("%s(?::%s)?", names.ApplicationSnippet, names.RelationSnippet)
anastasiamac
Sep 28, 2016
Member
Considering that this PR is about improving tests, I'll remove the validation that I have added for command arguments. All validation of application/endpoints happens at state as there are several permutations to consider and I do not want to hold up this PR from landing.
I'll adjust tests accordingly - just check that the correct number of arguments passed into the command.
| + addRelationFunc: func(endpoints ...string) (*params.AddRelationResults, error) { | ||
| + // At the moment, cmd implementation ignores the return values, | ||
| + // so nil is an acceptable return for testing purposes. | ||
| + return nil, nil |
axw
Sep 28, 2016
•
Edited 1 time
-
axw
Sep 28, 2016
Member
-
Sep 28, 2016
if you add a testing.Stub to the suite, and return .NextErr() here, you can get away without redefining s.mockAPI in the error test below, and you can use the .CheckCall method to check the args passed in the success case
| - ch = testcharms.Repo.CharmArchivePath(s.CharmsPath, "logging") | ||
| - err = runDeploy(c, ch, "lg", "--series", "quantal") | ||
| +func (s *AddRelationSuite) TestAddRelationSuccess(c *gc.C) { | ||
| + err := s.runAddRelation(c, "application1", "application2") | ||
| c.Assert(err, jc.ErrorIsNil) |
| @@ -61,25 +71,22 @@ func (c *removeRelationCommand) Init(args []string) error { | ||
| if len(args) != 2 { | ||
| return errors.Errorf("a relation must involve two applications") | ||
| } | ||
| + // None of the arguments can be empty. |
| +} | ||
| + | ||
| +func (s *RemoveRelationSuite) TestRemoveRelationSuccess(c *gc.C) { | ||
| + err := s.runRemoveRelation(c, "application1", "application2") |
| + } | ||
| +} | ||
| + | ||
| +func run(c *gc.C, command, expectedError string, args ...string) { |
axw
Sep 28, 2016
Member
I think this is a bit too generic to live in this file. Move to package_test.go, or make a suite method?
axw
Sep 28, 2016
Member
I think the calls of run would be more readable if you had a separate function for the error case.
anastasiamac
Sep 28, 2016
Member
Good idea \o/ I'll move this to the package file and will return error so that individual tests can check for failures if expected.
| + // return an error if we attempt to run two commands in the | ||
| + // same test. | ||
| + loggo.ResetWriters() | ||
| + ctx, err := cmd.DefaultContext() |
axw
Sep 28, 2016
Member
can we use coretesting.Context() instead? then you can use coretesting.Stderr(ctx) where you're currently asserting bytes.Buffer
anastasiamac
Sep 28, 2016
Member
Yes, the plan is that runJujuCommand will be dropped and this one will be used as a package-wide standard. I did not want to change all tests to the new version in this PR to minimise the diff. I'll do so in a follow up though.
anastasiamac
added some commits
Sep 28, 2016
|
I'm a bit confused by the github review pane. It still has 2 comments from Andrew, but only 2 out of all the comments made. Maybe because the change you made to address the comments happens below his actual comment, rather than to the line he commented on? Anyway, LGTM. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Intermittent :) |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
anastasiamac commentedSep 27, 2016
This PR:
I have consciously removed some tests like relation validation as those belong in the package where validation actually happens - see apiserver/application. And, indeed, we have the tests that cover all possible permutations there.