Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Make cmd more testable by abstracting authArgs and AzureClient #3756

Merged
merged 5 commits into from
Sep 4, 2018

Conversation

serbrech
Copy link
Member

What this PR does / why we need it:
Improve testability of the cmd package by abstracting the authArgs dependency.
This allows tests to stub the authArgs.getClient() call and return a fake ACSEngineClient instead of the concrete AzureClient instance

Special notes for your reviewer:
If this is ok, the same pattern can be applied for other command that depends on the AzureClient.

@acs-bot acs-bot added size/M and removed size/S labels Aug 28, 2018
@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #3756 into master will increase coverage by 0.31%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #3756      +/-   ##
==========================================
+ Coverage   55.48%   55.79%   +0.31%     
==========================================
  Files         108      108              
  Lines       16140    16143       +3     
==========================================
+ Hits         8955     9007      +52     
+ Misses       6420     6347      -73     
- Partials      765      789      +24

@acs-bot acs-bot added size/L and removed size/M labels Aug 29, 2018
@jackfrancis
Copy link
Member

/lgtm pending E2E, thanks @serbrech!

@jackfrancis
Copy link
Member

@serbrech FYI, this looks fine, but it occured to me that our E2E doesn't traverse bin/acs-engine deploy and instead uses bin/acs-engine generate + az group deployment create.

Let's use this as an excuse to build a 1st class bin/acs-engine deploy E2E test. Stand by... :)

@serbrech
Copy link
Member Author

added #3776

@serbrech serbrech mentioned this pull request Aug 31, 2018
4 tasks
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot
Copy link

acs-bot commented Sep 4, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, jackfrancis, serbrech

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [CecileRobertMichon,jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@CecileRobertMichon CecileRobertMichon merged commit ebd4bc3 into Azure:master Sep 4, 2018
@ghost ghost removed the in progress label Sep 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants