Refactor and add tests for Item client methods #119

joe94 · 2021-02-04T02:13:21Z

No description provided.

codecov · 2021-02-04T02:14:49Z

Codecov Report

Merging #119 (a09a088) into main (faedd84) will increase coverage by 11.31%.
The diff coverage is 69.11%.

@@             Coverage Diff             @@
##             main     #119       +/-   ##
===========================================
+ Coverage   43.19%   54.51%   +11.31%     
===========================================
  Files          16       16               
  Lines         801      842       +41     
===========================================
+ Hits          346      459      +113     
+ Misses        437      358       -79     
- Partials       18       25        +7

Impacted Files	Coverage Δ
chrysom/store.go	`0.00% <ø> (ø)`
store/provide.go	`0.00% <0.00%> (ø)`
chrysom/client.go	`73.94% <76.23%> (+73.94%)`	⬆️
store/transport.go	`85.57% <84.61%> (-1.03%)`	⬇️
store/inputValidation.go	`91.66% <100.00%> (+1.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faedd84...6198207. Read the comment docs.

kristinapathak

I'm pretty wary of putting pointers everywhere and I'm not sure what the benefits of these structs are.

kristinapathak · 2021-02-04T19:22:38Z

chrysom/client.go

+	ErrGetItemsFailure          = errors.New("failed to get items. Non-200 statuscode was received")
+	ErrRemoveItemFailure        = errors.New("failed to delete item. Non-200 statuscode was received")
+	ErrPushItemFailure          = errors.New("failed to push item. Non-success statuscode was received")


Some of these errors make sense to be exported, but some of them don't seem specific enough to be useful? Usually I export an error because the consumer of the package could have different logic/functionality after being given the error. Are all of these useful in that way?

I thought about making these more specific but they would blow up in number so I thought they'd still be helpful if returned in some form like this:

return nil, fmt.Errorf("statusCode %v: %w", response.Code, ErrPushItemFailure)

This will help when looking at tr1d1um logs and realizing Argus is returning a 403 instead of just a generic non-200 which I think is what you were getting at.

I have no problem with error variables; my point is all of them are exported. The only reason to export an error is to provide consumers of this package a way to do different logic based on receiving the error:

err := doSomething() // special flow if errors.Is(err, ErrNotFound) { doSomethingElse() return } // normal flow

kristinapathak · 2021-02-04T19:24:51Z

chrysom/client.go

+func NewClient(config *ClientConfig) (*Client, error) {
+	err := validateConfig(config)


This change means that now instead of validateConfig() modifying NewClient's copy of the config, it is modifying the config struct that the caller also has. We're good with that?

Yeah, I think that's fine. Users should make a defensive copy if they intend to reuse the config.

kristinapathak · 2021-02-04T19:29:47Z

chrysom/client.go

@@ -129,140 +162,229 @@ func validateConfig(config *ClientConfig) error {
 	}
 	return nil
 }
+func shouldUseJWTAcquirer(options acquire.RemoteBearerTokenAcquirerOptions) bool {


Could make this shorter by calling it isNonEmpty() or flip the logic and call it isEmpty().

you know, we could also just add an IsEmpty() function to acquire.RemoteBearerTokenAcquirerOptions....

chrysom/client.go

kristinapathak · 2021-02-04T21:15:44Z

chrysom/client.go


-	if config.Auth.Basic != "" {
-		return acquire.NewFixedAuthAcquirer(config.Auth.Basic)
+func validateGetItemsInput(input *GetItemsInput) error {


This is a personal preference, but the ordering of functions in this file make it more difficult to see how the pieces fit together. For example, this function is defined far away from the functions that use it. Maybe it belongs with the GetItemsInput struct? I had to go to another file to find out what we were validating.

Yeah, good call. I'll move these to the paramStructs file.

chrysom/client.go

kristinapathak · 2021-02-04T21:39:58Z

chrysom/client.go

+	request, err := c.makeRequest(input.Owner, http.MethodGet, URL, nil)
+	if err != nil {
+		return nil, err
 	}

-	items := []model.Item{}
-	err = json.NewDecoder(response.Body).Decode(&items)
+	response, err := c.do(request)
 	if err != nil {
 		return nil, err
 	}


with every makeRequest() call immediately followed by do() and neither having their error wrapped to provide more context, I don't see the point in having two separate functions? Can we just combine them into a function called sendRequest() with the parameters that makeRequest() takes?

I created them separately as I wasn't sure what would happen in between but I now I think combining makes sense 💯

kristinapathak · 2021-02-04T21:59:58Z

chrysom/client.go


-	response, err := c.client.Do(request)
+// GetItems fetches all items in a bucket that belong to a given owner.
+func (c *Client) GetItems(input *GetItemsInput) (*GetItemsOutput, error) {


Accepting this as a pointer allows the client to modify the *GetItemsInput. I know it isn't currently doing that, but it makes me nervous. What is the benefit of having a pointer here? Similarly, what is the benefit of having a struct? Why not take the two strings separately?

I pretty much have the same issue with other Input structs.

I followed a similar pattern to what folks at AWS use for their dynamodb go sdk https://docs.aws.amazon.com/sdk-for-go/api/service/dynamodb/#DynamoDB.PutItem

One big benefit is that it facilitates updating the client without changing the signature of the methods. We can deprecate new fields and add new ones in a backwards compatible way.

Even when our structs are not super large, PushItem(*PushItemInput)PushItemOutput already ran around 4000ns faster / op than PushItemOutput(PushItemInput)PushItemOutput

goos: darwin goarch: amd64 pkg: github.com/xmidt-org/argus/chrysom BenchmarkPushItemByValue-12 13467 80400 ns/op 7841 B/op 100 allocs/op PASS ok github.com/xmidt-org/argus/chrysom 2.140s goos: darwin goarch: amd64 pkg: github.com/xmidt-org/argus/chrysom BenchmarkPushItem-12 15087 77748 ns/op 7865 B/op 101 allocs/op PASS ok github.com/xmidt-org/argus/chrysom 2.248s

Everything I research about passing a pointer vs value says that value is faster:
https://medium.com/a-journey-with-go/go-should-i-use-a-pointer-instead-of-a-copy-of-my-struct-44b43b104963

I often find that pointers are used by SDKs that are written in multiple languages...it doesn't feel very Go-like to me. Writing functions in this way seems very different from how we currently write code, and if we want to do this I think it should be a team decision. Personally, I don't think the benefits of potential future backwards compatibility makes up for the potential for bugs, additional code maintenance, and larger mental model needed to understand the code (now I cannot simply look at the function signature and understand what's going on - I have to go look up what these structs are). But if this is the direction the team wants to move in, then we can.

*GetItemsOutput is a pointer to a struct that only has a slice, which is a pointer. Why can't we just return the slice?

The decision here is more about how the API will evolve. It feels to me like this method won't ever need to return anything but a slice, so have a full-on struct as a result type seems unnecessary. I'm not as close to the design, though.

Don't forget that you can take the middle ground and return a custom slice type:

type Items []Item func GetItemsOutput(...) Items

That has a few advantages:

(1) You can now introduce methods on your return type Items. That helps the API evolve over time, as you can introduce additional behavior without having to change the GetItemsOutput function.

(2) You can document the Items type with godoc

(3) You can switch to a type Items struct {...} in the future without too much of a breaking change. Since clients won't typically instantiate the Items type themselves, it won't break very much code.

chrysom/client.go

kristinapathak · 2021-02-04T22:08:47Z

chrysom/client.go


-	response, err := c.client.Do(request)
+// GetItems fetches all items in a bucket that belong to a given owner.
+func (c *Client) GetItems(input *GetItemsInput) (*GetItemsOutput, error) {


*GetItemsOutput is a pointer to a struct that only has a slice, which is a pointer. Why can't we just return the slice?

I have the same question about other Output structs.

Responded in comment above.

Passing by value seems better in this instance.

I would second what @kristinaspring says above. I'm not so much worried about speed of a particular call. Over time and at scale, the garbage collector performance tends to dominate. Passing by value removes the garbage collector from that part of the callstack, at least insofar as the parameters are concerned.

It's best to make the decision to pass by value or pointer based on API design rather than performance. If something is immutable or shouldn't be modified by a function, passing by value is a great way to communicate that without having to put a lot of verbiage in godoc. Alternatively, if the function does or may modify something, passing by pointer communicates that succinctly as well.

Obviously, if you benchmark and find significant performance gains by switching to pointers, by all means pass by pointer. But 4000ns/op, or in other words 4 microseconds/op, doesn't seem all that huge.

kcajmagic · 2021-02-04T22:52:50Z

chrysom/store.go


-	Start(ctx context.Context) error
+	Start(ctx context.Context, input *GetItemsInput) error


This no longer matches the uber.fx hook struct for starting up an application.
https://pkg.go.dev/go.uber.org/fx#Hook

Ah, good catch 👍

Co-authored-by: kristinaspring <kmspring57@gmail.com>

…to feature/betterTesting

kristinapathak

Looks great! 🚀

kristinapathak · 2021-02-09T04:25:11Z

chrysom/client.go

-		c.ticker.Stop()
+	if c.observer != nil && c.observer.ticker != nil {
+		c.observer.ticker.Stop()
+	}


This doesn't need to be done in this pr, but should we add a WaitGroup to ensure that the long running go routine has exited before returning? if GetItems() takes time but is successful, we could be trying to update the listener after it is already shut down.

Yeah, I think that's a great idea to avoid things sneakily happening in the background after Stop() returns 🚀
I'll add an issue for it

sonarqubecloud · 2021-02-09T17:21:52Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

joe94 linked an issue Feb 4, 2021 that may be closed by this pull request

Add unit tests to chyrsom #79

Closed

joe94 changed the title ~~Feature/better testing~~ Refactor and add tests for Item client methods Feb 4, 2021

joe94 requested review from kristinapathak and kcajmagic February 4, 2021 02:23

joe94 added this to the v0.3.10 milestone Feb 4, 2021

joe94 added 21 commits February 4, 2021 14:08

return exported errors

6904611

unit test for validateConfig

3808402

i/o structs for GetItems + helper make/do funcs

70252bb

add test for do function

c220ce1

add makeRequest tests

c9a81c4

Fix some typos

1629e66

cover obvious cases for client.do

b39cc98

Test GetItems

d22fd43

try new name for file

2b70aa7

Update pushItem method

0f0064b

Add PushItem test

4cf9492

Update removeItem method

42bf0c9

add removeItem tests

d7d238e

minor reformat and clean up in test file

5bf2a70

Move helper struct up in the file

b84cfa3

Add missing test case

d0746ca

Update existing code

94c6f45

Add comments to exported param structs

c836bd5

Add disclaimer note

df97c2e

fix linter errors and add missing test assertions

34ac37d

add release notes

5891bca

joe94 force-pushed the feature/betterTesting branch from 7372a61 to 5891bca Compare February 4, 2021 22:09

kristinapathak suggested changes Feb 4, 2021

View reviewed changes

joe94 linked an issue Feb 4, 2021 that may be closed by this pull request

Improve error handling in chrysom #107

Closed

kcajmagic reviewed Feb 4, 2021

View reviewed changes

joe94 and others added 4 commits February 5, 2021 09:15

fix missing errorf statement

543bcec

clean up old values

ef5ae75

Update chrysom/client.go

83d7eda

Co-authored-by: kristinaspring <kmspring57@gmail.com>

add changes from code review

a4b953c

joe94 modified the milestones: v0.3.10, v0.3.11 Feb 6, 2021

joe94 added 11 commits February 7, 2021 22:59

Merge branch 'feature/betterTesting' of github.com:xmidt-org/argus in…

406d6ae

…to feature/betterTesting

Merge make request and do helper funcs

29e3851

Update code around GetItems

3293bcc

update getItems test

ae98d19

update pushItem

a8f6431

Update removeItem

a509abd

revert start signature

a6e56ee

more clean up and updates

32df96e

minor clean up and standardize naming

8e37bf1

make some errors internal

9b0cf38

improve error handling/reporting

64510bf

kristinapathak approved these changes Feb 9, 2021

View reviewed changes

joe94 mentioned this pull request Feb 9, 2021

Add waitgroup for listeners #126

Closed

joe94 added 2 commits February 9, 2021 09:17

fix linting warnings

1e6f0c3

linting fix

6198207

joe94 merged commit 9d1bc42 into main Feb 9, 2021

joe94 deleted the feature/betterTesting branch February 9, 2021 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and add tests for Item client methods #119

Refactor and add tests for Item client methods #119

joe94 commented Feb 4, 2021

codecov bot commented Feb 4, 2021 •

edited

Loading

kristinapathak left a comment

kristinapathak Feb 4, 2021

joe94 Feb 5, 2021

kristinapathak Feb 5, 2021

kristinapathak Feb 4, 2021

joe94 Feb 5, 2021

kristinapathak Feb 4, 2021

kristinapathak Feb 4, 2021

joe94 Feb 5, 2021

kristinapathak Feb 4, 2021

joe94 Feb 5, 2021

kristinapathak Feb 4, 2021

kristinapathak Feb 4, 2021

joe94 Feb 5, 2021

kristinapathak Feb 5, 2021

johnabass Feb 5, 2021

kristinapathak Feb 4, 2021

kristinapathak Feb 4, 2021

joe94 Feb 5, 2021

johnabass Feb 5, 2021

kcajmagic Feb 4, 2021

joe94 Feb 5, 2021

kristinapathak left a comment

kristinapathak Feb 9, 2021

joe94 Feb 9, 2021

sonarqubecloud bot commented Feb 9, 2021

		func NewClient(config ClientConfig) (Client, error) {
		err := validateConfig(config)


		Start(ctx context.Context) error
		Start(ctx context.Context, input *GetItemsInput) error

Refactor and add tests for Item client methods #119

Refactor and add tests for Item client methods #119

Conversation

joe94 commented Feb 4, 2021

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

kristinapathak 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristinapathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 9, 2021

codecov bot commented Feb 4, 2021 •

edited

Loading