Lab7 - Errors #681
Lab7 - Errors #681
Conversation
jimbosoft
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe,
rokane and
samuong
as code owners
Codecov Report
@@ Coverage Diff @@
## master #681 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 288 292 +4
Lines 5967 6062 +95
=========================================
+ Hits 5967 6062 +95
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Some comments for you... I'll be back in a few months :)
It would be nice if you could squash your commits into a cohesive set. Leaving old broken commits is like renaming a function to foo_old(), fixing it as foo() and leaving foo_old() in the code.
Also, I edited your PR message to remove the space between # 682 - that way, github creates it as a link.
| @@ -0,0 +1,45 @@ | |||
| package main | |||
|
|
|||
| func (s *storesSuite) TestReadFailure() { | |||
There was a problem hiding this comment.
These test functions should be storer_test.go along with the other tests. Splitting it up like this just removes the context needed to understand the tests. Also, keeping the successful and failure tests together makes it easier to understand the unit under test from the test cases.
| package main | ||
|
|
||
| func (s *storesSuite) TestReadFailure() { | ||
| pup2, err := s.store.ReadPuppy(1) |
There was a problem hiding this comment.
Why is this pup2? Where is pup (pup1)? Same applies to the other tests in this file. You use pup2 when there is no original pup.
This matters because it can be confusing. When I saw pup2, I assumed that the test fixture was setting up pup1 for all the tests in the suite, so I went to look at that to see what pup1 was and could not find it. I'm stil not 100% that there is not a pup1 hiding somewhere else (another downside to splitting these tests into a different file - now I don't know how many files I need to look at to find what I'm looking for) and how it may affect these tests.
| pup2, err := s.store.ReadPuppy(1) | ||
| s.Require().Nil(pup2) | ||
| s.Require().Equal(ErrIDNotFound, err) | ||
| s.Require().NotEmpty(err.Error()) |
There was a problem hiding this comment.
This is an odd thing to test. If you want to test that the ErrIDNotFound presents as a non-empty string, that test is completely separate from the store test. Furthermore, why is "empty" the useful test for this? If the string was "skdjjfhaksdjafs" - would that be OK?
| @@ -0,0 +1,45 @@ | |||
| package main | |||
|
|
|||
| func (s *storesSuite) TestReadFailure() { | |||
There was a problem hiding this comment.
My style for naming tests would be to name this TestReadIDNotFound, and the next would be TestReadValueBelowZero. The tests share the same prefix (TestRead) so I know they are grouped, and the "failure" is specific about exactly which failure is being tested.
This is just my style though, but I think it helps.
07_errors/jimbotech/storer_test.go
Outdated
| id, err := s.store.CreatePuppy(&pup) | ||
| s.Require().NoError(err) | ||
| s.Require().NotEqual(pup.ID, uint32(1)) | ||
| s.Require().Equal(id, pup.ID, "Pup id must be set to actual id") |
There was a problem hiding this comment.
This require/assert belongs in TestCreateSuccess as that is an invariant of the CreatePuppy method. This function is a text fixture called from many places, which means if the storer returned a bad id such that this fails, every test that uses this fixture function will also fail even though what they are testing themselves may well be correct.
07_errors/jimbotech/sync_mapstore.go
Outdated
| func (s *SyncMapStore) CreatePuppy(p *Puppy) (int32, error) { | ||
| s.mux.Lock() | ||
| defer s.mux.Unlock() | ||
| p.ID = rand.Int31() |
There was a problem hiding this comment.
As above, the gravity of my wickedness is still a mystery to me ... added uniqueness check ...
07_errors/jimbotech/sync_mapstore.go
Outdated
| // SyncMapStore stores puppies threadsafe. | ||
| type SyncMapStore struct { | ||
| mux sync.Mutex | ||
| sync.Map |
There was a problem hiding this comment.
by embedding the map, you are making the map methods directly accessible to users of SyncMapStore, allowing them to bypass any checks/validation you have. You should put the sync.Map in as an unexported field.
07_errors/jimbotech/sync_mapstore.go
Outdated
| ) | ||
|
|
||
| // SyncMapStore stores puppies threadsafe. | ||
| type SyncMapStore struct { |
There was a problem hiding this comment.
the filename should be syncmapstore.go. But I would call the type SyncStore because shorter is better. From the outside, no-one cares that it is a map - it's a store. So don't name it with "Map". Which means the filename could be syncstore.go
07_errors/jimbotech/storer.go
Outdated
|
|
||
| const ( | ||
| // ErrNegativeID error number if id is negative | ||
| ErrNegativeID = -1 |
There was a problem hiding this comment.
Don't set the error code to -1. Just use iota and leave the assignment off subsequent error codes. That let's Go assign the numbers since the actual numbers themselves are not relevant.
There was a problem hiding this comment.
Interesting ... Done
07_errors/jimbotech/storer.go
Outdated
|
|
||
| // ErrNotConstructed returned if the interface was called without | ||
| // first constructing the underlaying structure. | ||
| var ErrNotConstructed = errors.New("store not created") |
There was a problem hiding this comment.
Why is this not an Error like the other ones?
There was a problem hiding this comment.
Because god willed it so ... however if you will something else, that's fine with me too. May the gods argue with each other ... Done
1.) Made sure that the items are copied when they are inserted and removed from the store 2.) Change the generation of the item id to using math.rand in order to always get a positiv int 3.) Removed the locking from the store that is not the sync store, as this was not what the instructions asked for
This is a quick effort to move forward with remaining PRs.
Add lab 6 - CRUD Puppy with interface
Add lab 7 - Errors
Add lab 5 - Stringer
Add lab 4 - Numeronym
Add lab 8 - Project Layout
Removed error_test and added functions into store_test.go Renamed sync_mapstore to syncstore.go Various other renamings and changes Added syncstore to lab 7
Removed error_test and added functions into store_test.go Renamed sync_mapstore to syncstore.go Various other renamings and changes Added syncstore to lab 7
|
Please merge or close. |
Fixes #682
Review of colleague's PR #
Changes proposed in this PR:
Lab 7 - adding error type and added mutex locking to the sync store
-