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

Add test cases for apis. #1290

Merged
merged 5 commits into from
Jun 24, 2020
Merged

Add test cases for apis. #1290

merged 5 commits into from
Jun 24, 2020

Conversation

harryge00
Copy link
Contributor

@harryge00 harryge00 commented Jan 19, 2020

What this PR does / why we need it:
Currently, GetPlanInProgress and NoPlanEverExecuted in instances_types has no unit tests. Added tests for them.

Fixes #

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

LGTM thanks :)

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

I left a few nits, but, otherwise lgtm! PTAL and we can 🚢 it

planStatus map[string]PlanStatus
expectedPlanName string
}{
{"no plan ever run", map[string]PlanStatus{"test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you please use named fields (e.g. name: "no plan ever run") for all test cases? It's getting harder to read for nested structs.

Comment on lines 121 to 123
if actualName != tt.expectedPlanName {
t.Errorf("%s: Expected to get plan %s but got plan status of %v", tt.name, tt.expectedPlanName, actual)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

simple assertion suffices:

assert.Equal(t, tt.expectedPlanName, actual)

Comment on lines 166 to 168
if actual != tt.expectedResult {
t.Errorf("%s: Expected to get plan %v but got %v", tt.name, tt.expectedResult, actual)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same here:

assert.Equal(t, tt. expectedResult, actual)

@kensipe
Copy link
Member

kensipe commented Mar 10, 2020

@harryge00 ping! what is next here?

@kensipe kensipe self-assigned this Mar 10, 2020
…master and updated code.

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe
Copy link
Member

kensipe commented Mar 10, 2020

Unable to interactive rebase to fix DCO. There is no DCO issue here, all code is modifications and additions to test which are KUDO specific and isn't possible to put us at risk.

@kensipe kensipe dismissed zen-dog’s stale review March 10, 2020 19:13

all pr review have been addressed

@gerred
Copy link
Member

gerred commented Mar 10, 2020

understood. squash and merge with Signed-off-by - this can be done with the Chrome extension. yay for being an admin!

@gerred
Copy link
Member

gerred commented Mar 10, 2020

@harryge00 please at some point confirm you can meet the https://developercertificate.org/ requirements with posting your sign off in this PR, otherwise we will need to re-write your work. thank you!

@kensipe
Copy link
Member

kensipe commented Mar 10, 2020

sure... shouldn't need a plugin to add a signoff to the merge commit (we just won't get a green build here)

@gerred
Copy link
Member

gerred commented Mar 10, 2020

@kensipe of course you don't, but it's an easy way to just add it in real quick. :)

@kensipe kensipe changed the base branch from master to main June 24, 2020 00:41
@kensipe kensipe merged commit 1d3ca2b into kudobuilder:main Jun 24, 2020
@kensipe kensipe added this to Done in KUDO Global via automation Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
KUDO Global
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants