Unit tests #1

pop · 2019-11-20T19:01:55Z

Unit Tests Initial Addition

The goal of this Pull Request is to add some basic "Happy Path" testing to the Golang SDK library.

While there are many refactors we are hoping to do in the coming weeks, this is just to create a baseline of test with minimal refactors to the actual API client.

Proposed changes

Verifying this change

To verify these changes please check off the following boxes:

Run go test ./cbclient/ at the base of the repository.
- Setup your go environment to use the latest version of Go (>=0.13.0).
- Clone the respository.
- The tests pass.
Review the changes to the code.

Administrivia

Everything below this line is just used for CloudBolt internal accounting.

Jira ID: DEV-12082

This commit includes a bunch of unit tests that will make it much easier to maintian and refactor the CloudBolt Go SDK going forward. In addition to the new unit tests there are a few changes to the API Client, really just code readability fixes. Lastly, there are some Docs improvements and a rationale about the what/why of our chosen tests design pattern. The tests are far from complete. I hope to increase testing coverage greatly when I have more cycles at my disposal.

[DEV-12082]

cbclient/api_client.go

nickbeaird

Feel free to take in my comments for now, and let's get your changes merged. I think that this looks great, and thank you for giving me time to start to review.

cbclient/api_client.go

nickbeaird · 2019-11-27T22:03:20Z

cbclient/api_client.go

+		Self         CloudBoltHALItem `json:"self"`
+		Owner        CloudBoltHALItem `json:"owner"`
+		Parent       CloudBoltHALItem `json:"parent"`
+		Subjobs      []interface{}    `json:"subjobs"`


I am not understanding why this interface is being used, or also how it is being used right now. I am looking at this article and seeing that the interfaces are a collection of methods, but I also see that it's set to an array. https://gobyexample.com/interfaces

Looking around it seems like Subjobs should be a []CloudBoltHALItem so I'm going to make that change. Good catch!

nickbeaird · 2019-11-27T22:11:33Z

cbclient/api_client.go

@@ -107,37 +106,21 @@ type CloudBoltOrder struct {
 	} `json:"items"`
 }

+// CloudBoltJob contains metadata about a Job.
+// Useful for getting the status of a running or completed job.
 type CloudBoltJob struct {


I am comparing the CloudBoltJob to that of a job on ProdCIT. There seems to be differences. https://prodcit.lab.iad.cloudboltsw.com/api/v2/jobs/10302/

I will check in soon, so that I am not comparing apples to oranges.

I 💯 think there are discrepancies between the API Client and the API itself. I've made a story (DEV-14020) to go through all of the big-ticket items and make sure the go struct representations of our API objects reflect reality for some supported version of yet-to-be-decided supported CloudBolt version.

nickbeaird · 2019-11-27T22:32:11Z

cbclient/api_client.go

-			PowerOn struct {
-				Href  string `json:"href"`
-				Title string `json:"title"`
-			} `json:"power_on,omitempty"`


I am wondering what is happening with these omitempty tags with an array. https://www.sohamkamani.com/blog/golang/2018-07-19-golang-omitempty/

OR
In reference to this post, is it that the array is empty, so it is not displayed? See the section "Values that Cannot be Ommitted"
golang/go#29310

Thanks for the blogpost, super helpful! My current theory is that I think this is actually a bug. The omitempty as it's written here will only have an effect when serializing the struct into JSON. For example, if Href and Title are empty ("") then PowerOn won't be included in the JSON output e.g., {"actions": [ ]}.
Now if we wanted to exclude this from the struct when de-serializing (parsing the JSON into a struct) we would want PowerOn to be a pointer. I got that from the section you pointed out "Values that Cannot be Omitted".

That's just a theory though and we'll want to experiment a bit before making any structural changes.

oh cool! Love it.

nickbeaird · 2019-11-27T22:58:43Z

cbclient/api_client.go

+		Group      CloudBoltHALItem   `json:"group"`
+		Owner      CloudBoltHALItem   `json:"owner"`
+		ApprovedBy CloudBoltHALItem   `json:"approved-by"`
+		Actions    CloudBoltHALItem   `json:"actions"`


Looking at ProdCIT and my local, it looks like this can be an array, similar to Jobs below it.

https://prodcit.lab.iad.cloudboltsw.com/api/v2/orders/1/

Very strange. I thought I'd actions as a HAL item, but looking around at our CIT and my personal CIT server it seems like every order follows this format:

... snip ... "actions": [ { "duplicate": { "href": "/api/v2/orders/##/actions/duplicate/", "title": "Duplicate 'Order id ##'" } } ], ... snip ...

I'll fix that, it seems like a genuine bug in the implementation.

cbclient/testData.go

cbclient/api_client_test.go

pop

Thanks @nickbeaird for bringing up a lot of awesome questions! I think you caught a few bugs that I can't wait to fix.

Since the focus of this PR was to keep things as unchanged as possible, and just test the client as is I would vote that we merge this PR and makes the bugfixes in #2.

Thoughts?

cbclient/testData.go

pop · 2019-12-02T16:58:29Z

cbclient/api_client.go

-			PowerOn struct {
-				Href  string `json:"href"`
-				Title string `json:"title"`
-			} `json:"power_on,omitempty"`


Thanks for the blogpost, super helpful! My current theory is that I think this is actually a bug. The omitempty as it's written here will only have an effect when serializing the struct into JSON. For example, if Href and Title are empty ("") then PowerOn won't be included in the JSON output e.g., {"actions": [ ]}.
Now if we wanted to exclude this from the struct when de-serializing (parsing the JSON into a struct) we would want PowerOn to be a pointer. I got that from the section you pointed out "Values that Cannot be Omitted".

That's just a theory though and we'll want to experiment a bit before making any structural changes.

cbclient/api_client_test.go

pop · 2019-12-02T17:26:24Z

cbclient/api_client.go

+		Group      CloudBoltHALItem   `json:"group"`
+		Owner      CloudBoltHALItem   `json:"owner"`
+		ApprovedBy CloudBoltHALItem   `json:"approved-by"`
+		Actions    CloudBoltHALItem   `json:"actions"`


Very strange. I thought I'd actions as a HAL item, but looking around at our CIT and my personal CIT server it seems like every order follows this format:

... snip ... "actions": [ { "duplicate": { "href": "/api/v2/orders/##/actions/duplicate/", "title": "Duplicate 'Order id ##'" } } ], ... snip ...

I'll fix that, it seems like a genuine bug in the implementation.

pop · 2019-12-02T17:27:54Z

cbclient/api_client.go

+		Self         CloudBoltHALItem `json:"self"`
+		Owner        CloudBoltHALItem `json:"owner"`
+		Parent       CloudBoltHALItem `json:"parent"`
+		Subjobs      []interface{}    `json:"subjobs"`


Looking around it seems like Subjobs should be a []CloudBoltHALItem so I'm going to make that change. Good catch!

cbclient/api_client.go

pop · 2019-12-02T17:29:42Z

cbclient/api_client.go

+		Self                  CloudBoltHALItem   `json:"self"`
+		Parent                CloudBoltHALItem   `json:"parent"`
+		Subgroups             []CloudBoltHALItem `json:"subgroups"`
+		Environments          []interface{}      `json:"environments"`


This should also be a []CloudBoltHALItem

pop · 2019-12-02T18:35:02Z

cbclient/api_client.go

@@ -107,37 +106,21 @@ type CloudBoltOrder struct {
 	} `json:"items"`
 }

+// CloudBoltJob contains metadata about a Job.
+// Useful for getting the status of a running or completed job.
 type CloudBoltJob struct {


I 💯 think there are discrepancies between the API Client and the API itself. I've made a story (DEV-14020) to go through all of the big-ticket items and make sure the go struct representations of our API objects reflect reality for some supported version of yet-to-be-decided supported CloudBolt version.

pop requested review from mrobbieb, nickbeaird and gsimore November 20, 2019 19:01

pop assigned nickbeaird Nov 20, 2019

pop mentioned this pull request Nov 22, 2019

Code Smell Changes #2

Merged

5 tasks

pop force-pushed the unit-tests branch from f4d857d to bb72d35 Compare November 22, 2019 22:51

Add unit test data for SubmitAction

c22c564

[DEV-12082]

nickbeaird reviewed Nov 27, 2019

View reviewed changes

cbclient/api_client.go Show resolved Hide resolved

nickbeaird approved these changes Nov 28, 2019

View reviewed changes

pop commented Dec 2, 2019

View reviewed changes

pop merged commit 897e209 into master Dec 2, 2019

pop deleted the unit-tests branch December 2, 2019 21:28

pop mentioned this pull request Dec 3, 2019

Update to CBClient v0.3.0 CloudBoltSoftware/terraform-provider-cloudbolt#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests #1

Unit tests #1

pop commented Nov 20, 2019 •

edited

Loading

nickbeaird left a comment

nickbeaird Nov 27, 2019

pop Dec 2, 2019

nickbeaird Nov 27, 2019

pop Dec 2, 2019

nickbeaird Nov 27, 2019

pop Dec 2, 2019

nickbeaird Dec 3, 2019

nickbeaird Nov 27, 2019

pop Dec 2, 2019

pop left a comment

pop Dec 2, 2019

pop Dec 2, 2019

pop Dec 2, 2019

pop Dec 2, 2019

pop Dec 2, 2019

Unit tests #1

Unit tests #1

Conversation

pop commented Nov 20, 2019 • edited Loading

Unit Tests Initial Addition

Proposed changes

Verifying this change

Administrivia

nickbeaird left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pop commented Nov 20, 2019 •

edited

Loading