Puppy store error handling #557
Conversation
runnerdave
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe and
rokane
as code owners
Codecov Report
@@ Coverage Diff @@
## master #557 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 102 106 +4
Lines 1748 1826 +78
=====================================
+ Hits 1748 1826 +78
Continue to review full report at Codecov.
|
runnerdave
force-pushed
the
puppy_store_error_handling
branch
from
7a6f16f to
4c5f9a9
Compare
07_errors/runnerdave/storer_test.go
Outdated
| r.NoError(err, "Should be able to read updated puppy") | ||
| assert.Equal(targetPuppy, updatedPuppy, "Updated puppy should be equal to puppy2") | ||
|
|
||
| // cleanup |
There was a problem hiding this comment.
Rather than requiring cleanup in each test, you should use SetupTest() and/or TeardownTest() to ensure a clean state. I suggest in your suite, you have a constant or function describing which sort of store to create and add a SetupTest() method to create the store fresh for each test. Then you can get rid of cleanup altogether and you know you can't get test failures because you didn't clean up properly at the end of a test.
There was a problem hiding this comment.
Hi @camh-anz, thanks a lot for your feedback I learned a lot implementing it, it should be ready now.
runnerdave
force-pushed
the
puppy_store_error_handling
branch
from
d6e4e0f to
7e28d16
Compare
|
|
||
| func TestStorer(t *testing.T) { | ||
| syncSuite := storerSuite{ | ||
| store: NewSyncStore(), |
There was a problem hiding this comment.
Since SetupTest() sets store at the start of every test, this does not need to be initialised here - you can just leave it as the zero value (nil).
| storerType: func() Storer { return NewSyncStore() }, | ||
| } | ||
| mapSuite := storerSuite{ | ||
| store: NewMapStore(), |
|
|
||
| // DeletePuppy deletes a puppy by id from the store | ||
| func (m *MapStore) DeletePuppy(id uint16) error { | ||
| if _, ok := m.puppies[id]; ok { |
There was a problem hiding this comment.
Usual Go style would be to test for !ok and return an error, leaving the non-error case at the left edge of the function: https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88
In ReadPuppy you couldn't really do that because of the scope of the puppy variable was the if block, but in this case you are ignoring that variable.
But this function is small enough that it doesn't really matter, so don't worry about changing it unless you really want to. Just keep it in mind in future - if it could go either way, keep the happy path at the left edge.
Fixes #103
Review of colleague's PR #513
Changes proposed in this PR: