Issue 46640043: code review 46640043: container/list: improve test coverage
|
Created:
12 years, 5 months ago by shawnps Modified:
12 years, 5 months ago Reviewers:
dave CC:
golang-codereviews, dave_cheney.net, gobot, bradfitz, gri Visibility:
Public. |
Descriptioncontainer/list: improve test coverage
Patch Set 1 #Patch Set 2 : diff -r 959a83dadbb9 https://code.google.com/p/go #Patch Set 3 : diff -r 959a83dadbb9 https://code.google.com/p/go #
Total comments: 11
Patch Set 4 : diff -r efdd955d766c https://code.google.com/p/go #Patch Set 5 : diff -r efdd955d766c https://code.google.com/p/go #
Total comments: 13
Patch Set 6 : diff -r cc85f5964821 https://code.google.com/p/go #
Total comments: 6
Patch Set 7 : diff -r 76ba983c4f3d https://code.google.com/p/go #MessagesTotal messages: 17
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
On 2013/12/31 06:26:09, shawnps wrote: > Hello mailto:golang-codereviews@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go LGTM but I never use list.List so please wait for someone else to review this.
Sign in to reply to this message.
R=golang-dev (assigned by dave@cheney.net)
Sign in to reply to this message.
On 2013/12/31 12:06:58, gobot wrote: > R=golang-dev (assigned by mailto:dave@cheney.net) This might be adding too much code for a small amount of logic, happy to abandon it if others think so as well.
Sign in to reply to this message.
https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... File src/pkg/container/list/list_test.go (right): https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:290: func TestLazyInit(t *testing.T) { TestZeroList maybe? https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:295: t.Errorf("Len() after PushFront with uninitialized List should be 1; got: %d", got) "after PushFront, Len = %d; want 1", got (the test name says the rest, drop parens on Len, got before want) Here and other cases too. https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:314: func insertBeforeAndAfterSetup() (*List, Element) { just inline this in the tests below. I'd rather see a few extra lines than try to mentally follow what this little func is supposed to be doing https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:327: l.InsertBefore(1, &mark) new(Element) instead of &mark
Sign in to reply to this message.
R=gri@golang.org (assigned by r@golang.org)
Sign in to reply to this message.
Please address bradfitz's feedback. https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... File src/pkg/container/list/list_test.go (right): https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:290: func TestLazyInit(t *testing.T) { On 2014/01/02 18:23:54, bradfitz wrote: > TestZeroList maybe? +1 https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:314: func insertBeforeAndAfterSetup() (*List, Element) { On 2014/01/02 18:23:54, bradfitz wrote: > just inline this in the tests below. I'd rather see a few extra lines than try > to mentally follow what this little func is supposed to be doing +1 https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:327: l.InsertBefore(1, &mark) On 2014/01/02 18:23:54, bradfitz wrote: > new(Element) instead of &mark +1
Sign in to reply to this message.
https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... File src/pkg/container/list/list_test.go (right): https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:290: func TestLazyInit(t *testing.T) { On 2014/01/02 18:23:54, bradfitz wrote: > TestZeroList maybe? Done. https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:295: t.Errorf("Len() after PushFront with uninitialized List should be 1; got: %d", got) On 2014/01/02 18:23:54, bradfitz wrote: > "after PushFront, Len = %d; want 1", got > > (the test name says the rest, drop parens on Len, got before want) > > Here and other cases too. Done. https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:314: func insertBeforeAndAfterSetup() (*List, Element) { On 2014/01/02 18:23:54, bradfitz wrote: > just inline this in the tests below. I'd rather see a few extra lines than try > to mentally follow what this little func is supposed to be doing Done. https://codereview.appspot.com/46640043/diff/40001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:327: l.InsertBefore(1, &mark) On 2014/01/02 18:23:54, bradfitz wrote: > new(Element) instead of &mark Done.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net, gobot@golang.org, bradfitz@golang.org, gri@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Some more. Looks good otherwise. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... File src/pkg/container/list/list_test.go (right): https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:291: var l1, l2, l3, l4 List one var decl immediately before the test makes this clearer var l1 List l1.PushFront(1) checklist(t, l1, []interface{1}) var l2 List ... https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:294: if got := l1.Len(); got != 1 { replace these if's with checkList - less code and more comprehensive test https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:314: // Test that l is not modified when calling InsertBefore with mark not an element of l // Test that a list l is not modified when calling InsertBefore with a mark that is not an element of l. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:315: func TestInsertBeforeMarkNotInL(t *testing.T) { TestInsertBeforeUnknownMark https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:324: // Test that l is not modified when calling InsertAfter with mark not an element of l // Test that a list l is not modified when calling InsertAfter with a mark that is not an element of l. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:325: func TestInsertAfterMarkNotInL(t *testing.T) { TestInsertAfterUnknownMark
Sign in to reply to this message.
https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... File src/pkg/container/list/list_test.go (right): https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:291: var l1, l2, l3, l4 List On 2014/01/03 19:45:35, gri wrote: > one var decl immediately before the test makes this clearer > > var l1 List > l1.PushFront(1) > checklist(t, l1, []interface{1}) > > var l2 List > ... Since checkList takes *List, I'll do var l1 = new(List), is that fine? https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:291: var l1, l2, l3, l4 List On 2014/01/03 19:45:35, gri wrote: > one var decl immediately before the test makes this clearer > > var l1 List > l1.PushFront(1) > checklist(t, l1, []interface{1}) > > var l2 List > ... Done. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:294: if got := l1.Len(); got != 1 { On 2014/01/03 19:45:35, gri wrote: > replace these if's with checkList - less code and more comprehensive test Done. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:314: // Test that l is not modified when calling InsertBefore with mark not an element of l On 2014/01/03 19:45:35, gri wrote: > // Test that a list l is not modified when calling InsertBefore with a mark that > is not an element of l. Done. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:315: func TestInsertBeforeMarkNotInL(t *testing.T) { On 2014/01/03 19:45:35, gri wrote: > TestInsertBeforeUnknownMark Done. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:324: // Test that l is not modified when calling InsertAfter with mark not an element of l On 2014/01/03 19:45:35, gri wrote: > // Test that a list l is not modified when calling InsertAfter with a mark that > is not an element of l. Done. https://codereview.appspot.com/46640043/diff/80001/src/pkg/container/list/lis... src/pkg/container/list/list_test.go:325: func TestInsertAfterMarkNotInL(t *testing.T) { On 2014/01/03 19:45:35, gri wrote: > TestInsertAfterUnknownMark Done.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net, gobot@golang.org, bradfitz@golang.org, gri@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2014/01/04 04:03:40, shawnps wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:dave@cheney.net, mailto:gobot@golang.org, > mailto:bradfitz@golang.org, mailto:gri@golang.org (cc: mailto:golang-codereviews@googlegroups.com), > > Please take another look. LGTM once these minor comments are addressed
Sign in to reply to this message.
https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... File src/pkg/container/list/list_test.go (right): https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... src/pkg/container/list/list_test.go:310: l := New() var l List https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... src/pkg/container/list/list_test.go:315: checkList(t, l, []interface{}{1, 2, 3}) checkList(t, &l, []interface{}{1,2,3}) https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... src/pkg/container/list/list_test.go:325: checkList(t, l, []interface{}{1, 2, 3}) same
Sign in to reply to this message.
https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... File src/pkg/container/list/list_test.go (right): https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... src/pkg/container/list/list_test.go:310: l := New() On 2014/01/04 09:40:21, dfc wrote: > var l List Done. https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... src/pkg/container/list/list_test.go:315: checkList(t, l, []interface{}{1, 2, 3}) On 2014/01/04 09:40:21, dfc wrote: > checkList(t, &l, []interface{}{1,2,3}) Done. https://codereview.appspot.com/46640043/diff/100001/src/pkg/container/list/li... src/pkg/container/list/list_test.go:325: checkList(t, l, []interface{}{1, 2, 3}) On 2014/01/04 09:40:21, dfc wrote: > same Done.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net, gobot@golang.org, bradfitz@golang.org, gri@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=36d5461843ce *** container/list: improve test coverage R=golang-codereviews, dave, gobot, bradfitz, gri CC=golang-codereviews https://codereview.appspot.com/46640043 Committer: Dave Cheney <dave@cheney.net>
Sign in to reply to this message.
|
