-
Notifications
You must be signed in to change notification settings - Fork 494
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 resources commands sanity check. #7691
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small things
charmOne := s.AddTestingCharm(c, s.charmName) | ||
|
||
var err error | ||
s.appOne, err = s.State.AddApplication(state.AddApplicationArgs{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the factory is preferable here since there's no error checking required - the factory does it. Plus it's the preferred way of creating test entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch \o/
It actually means that I now get a result for 'juju resources ' :D Of course, the downside of this is that I now discovered another command which output needs to be ordered to produce deterministic output.
I'll propose it as a separate PR and land it first prior to this one.
} | ||
|
||
func (s *stubCharmStore) ListResources(charms []charmstore.CharmID) ([][]charmresource.Resource, error) { | ||
s.stub.AddCall("ListResources", charms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the call is being recorded, it should be checked in a test somewhere.
Doing so would be a light weight way of making these tests a little more comprehensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is done at line 106...
!!build!! |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Description of change
This is a basic sanity check, desired for test coverage.
Historically, resources commands where not tested for full-stack connectivity. This omission caused several slippages during refactoring and improvements of juju commands.
With the addition of a basic feature test, these slippages should no longer occur.
This test does not test the actual functionality and behavior of the commands. It traverses full stack to ensure all components are wired in.
QA steps
Feature test runs successfully.
Documentation changes
n/a, internal change
Bug reference
Final part for https://bugs.launchpad.net/juju/+bug/1706809