Implement CRUD puppy with interface #538

patrickmarabeas · 2019-07-04T12:23:13Z

Adds the Puppy, Storer, MapStore and SyncStore type definitions.

MapStore implements the Storer interface and supplies a NewMapStore factory function.
SyncStore implements the Storer interface and supplies a NewSyncStore factory function.
Uuid increment is handled internally by MapStore Create
Read will return an empty Puppy struct if provided uuid doesn't exist
Update will return the given Puppy struct if the uuid doesn't exist
Destroy returns a bool dependent on uuid existing

Fixes #537

Review of colleague's PR #536

pentaphobe

After a while staring at your code (and adding a few extra tests) I cannot work out what’s happening to your coverage.

However one thing which may help is if you conform to the go style of returning an error from functions which have an error condition and testing against that.

Still don't think that should matter, but never know... (also worth mentioning just as a stylistic nitpick)

FWIW: I sometimes use the HTML view of coverage to help see what the tool's seeing, but in this case it told me very little.

Either way, this is the command I used:

go test -v -race -coverprofile /tmp/cover.out -covermode=atomic ./... && go tool cover -html /tmp/cover.out -o /tmp/profile.html && open /tmp/profile.html

FWIW 2: Tests I tried out were things like:

doing non deep-equals checks of zero value returns
calling the offending functions on a nil store (which obviously just crashes - alas precluding any coverage result. Whilst it's arguable that one might need to check for such things, surely that would cause coverage issues on your other functions too)
started adding a custom error to your code (for my own edification) but the task exploded - and was discarded - once I realised I'd have to update signatures and calls across most files and it's 2am :)

patrickmarabeas · 2019-07-05T00:04:05Z

Hopefully it's not error related - as I removed all my error handling upon seeing Lab 07 outline!

06_puppy/patrickmarabeas/mapStore_test.go

06_puppy/patrickmarabeas/syncStore.go

06_puppy/patrickmarabeas/mapStore.go

06_puppy/patrickmarabeas/syncStore_test.go

juliaogris · 2019-07-25T01:51:46Z

Please fix broken build and address @anzdaddy 's comments

06_puppy/patrickmarabeas/store_test.go

codecov · 2019-08-19T05:27:17Z

Codecov Report

Merging #538 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #538   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         182    185    +3     
  Lines        3446   3494   +48     
=====================================
+ Hits         3446   3494   +48

Impacted Files	Coverage Δ
06_puppy/patrickmarabeas/mapStore.go	`100% <100%> (ø)`
06_puppy/patrickmarabeas/main.go	`100% <100%> (ø)`
06_puppy/patrickmarabeas/syncStore.go	`100% <100%> (ø)`

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 5fee262...5d53a52. Read the comment docs.

anzboi

Great work, Just one think I would like to fix up.
From an interface level, we don't care about what ids are being created, only that they are different for any two Create calls. IMO the Create test should just create two puppies and check the generated ID's are different.
Feel free to disagree and I'll approve.

06_puppy/patrickmarabeas/types.go

anzboi · 2019-08-22T12:36:48Z

06_puppy/patrickmarabeas/types.go

+}
+
+type SyncStore struct {
+	uuid int


Interesting interpretation of uuid you have there.

Changed to uint

anzboi · 2019-08-22T12:39:43Z

06_puppy/patrickmarabeas/store_test.go

+	a := assert.New(suite.T())
+	id := suite.store.Create(Puppy{Breed: "Boxer", Color: "Brown", Value: "300"})
+
+	a.Equal(id, 1)


I think what @anzdaddy was getting at here is that TestCreate should create 2 puppies, and check the second ones ID is incremented by 1 from the first, or at least different, think about what could be generating these id's if you don't know what the implementation is.
Otherwise, great tests.

06_puppy/patrickmarabeas/types.go

06_puppy/patrickmarabeas/syncStore.go

Adds the Puppy, Storer and MapStore type definitions. MapStore implements the Storer interface and supplies a NewMapStore factory function. - Uuid increment is handled internally by MapStore Create - Read will return an empty Puppy struct if provided uuid doesn't exist - Update returns a bool whether a matching identifier was modified - Destroy returns a bool whether a matching identifier was modified Lab 06 (anz-bank#537)

SyncStore implements the Storer interface and supplies a NewSyncStore factory function. - Uuid increment is handled internally by SyncStore Create - Read will return an empty Puppy struct if provided uuid doesn't exist - Update returns a bool whether a matching identifier was modified - Destroy returns a bool whether a matching identifier was modified Lab 06 (anz-bank#537)

Add empty main function and tests to pass build requirements

rokane

Looking really good! Great suite of tests and really clean, well documented code. I have left a couple of comments throughout which I think would be nice to have. Also there is the empty main function, if you feel against what I have suggested then let me know, otherwise it would be nice to see something in there (even if it's simple)

rokane · 2019-08-26T01:19:52Z

06_puppy/patrickmarabeas/main.go

+package main
+
+func main() {
+


I find it a little strange seeing a blank main program. I know the description of the lab doesn't state what you need to include in the main, so you have technically ticked all the boxes, but I feel it makes sense to maybe create a puppy or something to give context to the work being done. Happy to hear your thoughts otherwise.

rokane · 2019-08-26T01:22:47Z

06_puppy/patrickmarabeas/mapStore.go

@@ -0,0 +1,56 @@
+package main
+
+type MapStore struct {


Would be nice to see some documentation on the MapStore struct as well, since it is a part of the exported package.

rokane · 2019-08-26T01:25:26Z

06_puppy/patrickmarabeas/syncStore.go

+	"sync"
+)
+
+type SyncStore struct {


Again here with the documentation as well

rokane · 2019-08-26T01:26:32Z

06_puppy/patrickmarabeas/types.go

+	Value string
+}
+
+type Storer interface {


Again here with the documentation.

rokane · 2019-08-26T01:28:47Z

06_puppy/patrickmarabeas/store_test.go

+
+func (suite *StoreSuite) TestReadNonExistent() {
+	a := assert.New(suite.T())
+	success := suite.store.Read(100)


success seems like a strange name for this variable. Maybe notFound or something which indicates that it didn't exist?

juliaogris · 2020-02-08T03:01:08Z

The go-course is now closed. Thank you very much for participating.

patrickmarabeas added Ready for review lab6 labels Jul 4, 2019

patrickmarabeas requested review from anzboi, anzdaddy, camscale, joshcarp, juliaogris, linuxmonk, pentaphobe and rokane as code owners July 4, 2019 12:23

patrickmarabeas changed the title ~~Lab 06 pm~~ Implement CRUD puppy with interface Jul 4, 2019

patrickmarabeas force-pushed the lab-06-pm branch from 2f62c1a to 3ed57a3 Compare July 4, 2019 13:12

pentaphobe reviewed Jul 4, 2019

View reviewed changes

anzdaddy requested changes Jul 6, 2019

View reviewed changes

juliaogris added Changes requested and removed Ready for review labels Jul 25, 2019

patrickmarabeas force-pushed the lab-06-pm branch from 3ed57a3 to 81569b6 Compare August 19, 2019 04:36

patrickmarabeas requested a review from samuong as a code owner August 19, 2019 04:36

patrickmarabeas force-pushed the lab-06-pm branch from 81569b6 to a35ef1f Compare August 19, 2019 05:06

golangcibot reviewed Aug 19, 2019

View reviewed changes

06_puppy/patrickmarabeas/store_test.go Outdated Show resolved Hide resolved

patrickmarabeas force-pushed the lab-06-pm branch 2 times, most recently from 33a306c to 1229a14 Compare August 19, 2019 05:21

patrickmarabeas added Ready for review and removed Changes requested labels Aug 19, 2019

patrickmarabeas requested a review from anzdaddy August 19, 2019 05:28

anzboi added Under review and removed Ready for review labels Aug 22, 2019

anzboi requested changes Aug 22, 2019

View reviewed changes

anzboi added Changes requested and removed Under review labels Aug 22, 2019

patrickmarabeas force-pushed the lab-06-pm branch from 1229a14 to 4b0de14 Compare August 25, 2019 05:42

golangcibot reviewed Aug 25, 2019

View reviewed changes

06_puppy/patrickmarabeas/types.go Outdated Show resolved Hide resolved

06_puppy/patrickmarabeas/syncStore.go Show resolved Hide resolved

patrickmarabeas added 3 commits August 25, 2019 15:50

Add empty main function

5d53a52

Add empty main function and tests to pass build requirements

patrickmarabeas force-pushed the lab-06-pm branch from 4b0de14 to 5d53a52 Compare August 25, 2019 05:51

patrickmarabeas requested a review from anzboi August 25, 2019 06:02

patrickmarabeas added Ready for review and removed Changes requested labels Aug 25, 2019

rokane added the Under review label Aug 26, 2019

rokane requested changes Aug 26, 2019

View reviewed changes

rokane added Changes requested and removed Ready for review Under review labels Aug 26, 2019

juliaogris closed this Feb 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CRUD puppy with interface #538

Implement CRUD puppy with interface #538

patrickmarabeas commented Jul 4, 2019

pentaphobe left a comment •

edited

Loading

patrickmarabeas commented Jul 5, 2019

juliaogris commented Jul 25, 2019

codecov bot commented Aug 19, 2019 •

edited

Loading

anzboi left a comment

anzboi Aug 22, 2019

patrickmarabeas Aug 25, 2019

anzboi Aug 22, 2019

patrickmarabeas Aug 25, 2019

rokane left a comment

rokane Aug 26, 2019

rokane Aug 26, 2019

rokane Aug 26, 2019

rokane Aug 26, 2019

rokane Aug 26, 2019

juliaogris commented Feb 8, 2020

		package main

		func main() {

Implement CRUD puppy with interface #538

Implement CRUD puppy with interface #538

Conversation

patrickmarabeas commented Jul 4, 2019

pentaphobe left a comment • edited Loading

Choose a reason for hiding this comment

patrickmarabeas commented Jul 5, 2019

juliaogris commented Jul 25, 2019

codecov bot commented Aug 19, 2019 • edited Loading

Codecov Report

anzboi 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

rokane 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

juliaogris commented Feb 8, 2020

pentaphobe left a comment •

edited

Loading

codecov bot commented Aug 19, 2019 •

edited

Loading