Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lab7 - Errors #681

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions 07_errors/jimbotech/error_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package main

func (s *storesSuite) TestReadFailure() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed

pup2, err := s.store.ReadPuppy(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

s.Require().Nil(pup2)
s.Require().Equal(ErrIDNotFound, err)
s.Require().NotEmpty(err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

}

func (s *storesSuite) TestNegReadFailure() {
pup2, err := s.store.ReadPuppy(-1)
s.Require().Nil(pup2)
s.Require().Equal(ErrValueBelowZero, err)
}

func (s *storesSuite) TestUpdateError() {
create(s)
pup2 := Puppy{Breed: "kelpie", Colour: "black", Value: "indispensable"}
err := s.store.UpdatePuppy(1, &pup2)
s.Require().Equal(ErrIDNotFound, err)
}

func (s *storesSuite) TestNegUpdateError() {
create(s)
pup2 := Puppy{Breed: "kelpie", Colour: "black", Value: "indispensable"}
err := s.store.UpdatePuppy(-1, &pup2)
s.Require().Equal(ErrValueBelowZero, err)
}

func (s *storesSuite) TestNegDeleteFailure() {
err := s.store.DeletePuppy(-1)
s.Require().Equal(ErrValueBelowZero, err)
}

func (s *storesSuite) TestDeleteFailure() {
err := s.store.DeletePuppy(1)
s.Require().Equal(ErrIDNotFound, err)
}

func (s *storesSuite) TestError() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be called TestErrorNil or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed

var err = &Error{}
err = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two lines can be replaced with var err *Error - you specify the type and by not initialising it, you get the zero value, which is nil.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

res := err.Error()
s.Assert().Equal("<nil>", res)
}
75 changes: 75 additions & 0 deletions 07_errors/jimbotech/mapstore.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package main

import (
"math/rand"
)

// MapStore stores puppies.
type MapStore map[int32]*Puppy

// length used for testing.
func (s MapStore) length() int {
return len(s)
}

// CreatePuppy add a puppy to storage
// choose a unique member id that is currently not in the collection
// but will modify the member ID.
func (s MapStore) CreatePuppy(p *Puppy) (int32, error) {
if s == nil {
return 0, ErrNotConstructed
}
for notUnique := true; notUnique; p.ID = rand.Int31() {
_, notUnique = s[p.ID]
}
sp := *p
s[p.ID] = &sp
return p.ID, nil
}

// ReadPuppy retrieve your puppy.
func (s MapStore) ReadPuppy(id int32) (*Puppy, error) {
if s == nil {
return nil, ErrNotConstructed
}
if id < 0 {
return nil, ErrValueBelowZero
}
val, found := s[id]
if !found {
return nil, ErrIDNotFound
}
retVal := *val
return &retVal, nil
}

// UpdatePuppy update your puppy store.
func (s MapStore) UpdatePuppy(id int32, puppy *Puppy) error {
if s == nil {
return ErrNotConstructed
}
if _, err := s.ReadPuppy(id); err != nil {
return err
}
puppy.ID = id
sp := *puppy
s[id] = &sp
return nil
}

// DeletePuppy remove the puppy from store.
func (s MapStore) DeletePuppy(id int32) error {
if s == nil {
return ErrNotConstructed
}
if _, err := s.ReadPuppy(id); err != nil {
return err
}
delete(s, id)
return nil
}

// NewMapStore constructor creates the map.
func NewMapStore() MapStore {
return MapStore{}
}
23 changes: 23 additions & 0 deletions 07_errors/jimbotech/mapstore_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package main

import (
"testing"

"github.com/stretchr/testify/assert"
)

// TestMapStoreWithoutContructor may not be required as it may not be
// possible to create an instance of that type without using the constructor
func TestMapStoreWithoutContructor(t *testing.T) {
var puppyStore MapStore
pup := Puppy{1, "kelpie", "brown", "indispensable"}

_, err := puppyStore.CreatePuppy(&pup)
assert.Equal(t, ErrNotConstructed, err)
err = puppyStore.UpdatePuppy(1, &pup)
assert.Equal(t, ErrNotConstructed, err)
_, err = puppyStore.ReadPuppy(1)
assert.Equal(t, ErrNotConstructed, err)
err = puppyStore.DeletePuppy(1)
assert.Equal(t, ErrNotConstructed, err)
}
18 changes: 18 additions & 0 deletions 07_errors/jimbotech/puppy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package main

import (
"fmt"
"io"
"os"
)

var out io.Writer = os.Stdout

func main() {
var puppyStore Storer = NewMapStore()
pup := Puppy{Breed: "kelpie", Colour: "brown", Value: "indispensable"}
id, _ := puppyStore.CreatePuppy(&pup)
if pup, err := puppyStore.ReadPuppy(id); err == nil {
fmt.Fprintf(out, "retrieved: %v %v %v\n", pup.Breed, pup.Colour, pup.Value)
}
}
17 changes: 17 additions & 0 deletions 07_errors/jimbotech/puppy_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package main

import (
"bytes"
"testing"
)

func TestMain(t *testing.T) {
expected := "retrieved: kelpie brown indispensable\n"
var buf bytes.Buffer
out = &buf
main()
actual := buf.String()
if actual != expected {
t.Errorf("expected %v, actual %v", expected, actual)
}
}
58 changes: 58 additions & 0 deletions 07_errors/jimbotech/storer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package main

import (
"strconv"
)

// Storer defines standard CRUD operations for Puppy
type Storer interface {
CreatePuppy(p *Puppy) (int32, error)
ReadPuppy(ID int32) (*Puppy, error)
UpdatePuppy(ID int32, Puppy *Puppy) error
DeletePuppy(ID int32) error
}

// Puppy stores puppy details.
type Puppy struct {
ID int32
Breed string
Colour string
Value string
}

// mapTest used during testing to verify underlaying map changes
type mapTest interface {
length() int
}

// Error is a custom error
type Error struct {
Message string
Code int
}

func (e *Error) Error() string {
if e == nil {
return "<nil>"
}
return e.Message + " Error Code: " + strconv.Itoa(e.Code)
}

const (
// ecNegativeID error number if id is negative
ecNegativeID = iota
// ecNotFound error number if id not found
ecNotFound = iota
// ecNotConstructed if the interface was called without being constructed first
ecNotConstructed = iota
)

// ErrValueBelowZero error generated if the calu is below zero
var ErrValueBelowZero = &Error{Message: "id below 0", Code: ecNegativeID}

// ErrIDNotFound error if the requested ID is not in the store
var ErrIDNotFound = &Error{Message: "id not found", Code: ecNotFound}

// ErrNotConstructed returned if the interface was called without
// first constructing the underlaying structure.
var ErrNotConstructed = &Error{Message: "store not created", Code: ecNotConstructed}
Loading