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

cmd/juju/application: make deploy init tests assume less about internals #7274

Merged
merged 1 commit into from Apr 26, 2017

Conversation

rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Apr 25, 2017

The tests are doing a deep-equal comparison of the deploy
command, which assumes more than it should about what
the state of the embedded objects. Make the code check
the fields it cares about explicitly.

Also clean up things a little bit:

  • NewDeployCommand is now the standard constructor, consistent
    with the other commands.
  • use runDeploy throughout, losing the hard-to-remember distinction
    between runDeploy and runDeployCommand.

QA check that the deploy command works.

@rogpeppe rogpeppe force-pushed the 150-deploy-test-assume-less branch from 761c889 to aaeffe9 Compare April 25, 2017 09:53
@rogpeppe
Copy link
Contributor Author

!!build!!

Copy link
Contributor

@mhilton mhilton left a comment

Choose a reason for hiding this comment

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

LGTM

@nskaggs
Copy link
Contributor

nskaggs commented Apr 25, 2017

!!build!!

Copy link
Contributor

@babbageclunk babbageclunk left a comment

Choose a reason for hiding this comment

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

Thanks, this seems much nicer.

The tests are doing a deep-equal comparison of the deploy
command, which assumes more than it should about what
the state of the embedded objects. Make the code check
the fields it cares about explicitly.

Also clean up things a little bit:
- NewDeployCommand is now the standard constructor, consistent
with the other commands.
- use runDeploy throughout, losing the hard-to-remember distinction
between runDeploy and runDeployCommand.
@rogpeppe rogpeppe force-pushed the 150-deploy-test-assume-less branch from aaeffe9 to 78cd4a0 Compare April 26, 2017 11:50
@rogpeppe
Copy link
Contributor Author

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Apr 26, 2017

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

@jujubot jujubot merged commit 0548895 into juju:develop Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants