Skip to content

Conversation

@eschwartz
Copy link
Contributor

@eschwartz eschwartz commented Feb 25, 2020

Proposed changes

Fixes #77

Add basic integration test coverage around dce leases create

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (changes to code, which do not change application behavior)

Checklist

  • I have filled out this PR template
  • I have read the CONTRIBUTING doc
  • I have added automated tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (README.md, inline comments, etc.)
  • I have updated the CHANGELOG.md under a ## next release, with a short summary of my changes

Relevant Links

var apiClient utl.APIer
var out observ.OutputWriter
var ApiClient utl.APIer
var Out observ.OutputWriter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to make these public, so I can mock them in tests
(our lease service relies on global variables)

expiry, err := s.Util.ExpandEpochTime(expiresOn)

if err != nil && expiry > 0 {
if err == nil && expiry > 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👆 Here's the bug

Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh...

Nicely done.

)

func TestLeasesCreate(t *testing.T) {
cli := NewCLITest(t)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put together a quick happy-path test for dce leases create. Could/should be expanded in the future, to cover other use cases

}
params.SetTimeout(5 * time.Second)
_, err := apiClient.PostAccounts(params, nil)
_, err := ApiClient.PostAccounts(params, nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to make these public, so I can mock them in tests
(our lease service relies on global variables)

Copy link
Contributor

@nathanagood nathanagood left a comment

Choose a reason for hiding this comment

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

LGTM; nice work.

expiry, err := s.Util.ExpandEpochTime(expiresOn)

if err != nil && expiry > 0 {
if err == nil && expiry > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh...

Nicely done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Creating an account with a expiration parameter doesn't seem to work correctly

2 participants