Improve coverage of WorkItemType and WorkItem #240

kwk · 2016-09-12T09:33:39Z

Click here and select the Files tab to see how much the coverage is increased per directory or file with this PR.

Fixes #216
Fixes #217

kwk · 2016-09-13T13:09:18Z

So far, the overall (unit+integration) coverage is bumped from 23.6% to 28.3% with the current tests. It may not seem like much but it is tedious.

kwk · 2016-09-15T06:38:11Z

So far, the overall (unit+integration) coverage is bumped from 23.6% to 33.0% with the current tests.

codecov-io · 2016-09-16T11:32:17Z

Current coverage is 47.67% (diff: 100%)

Merging #240 into master will increase coverage by 10.81%

@@             master       #240   diff @@
==========================================
  Files            32         33     +1   
  Lines          1199       1204     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            442        574   +132   
+ Misses          692        589   -103   
+ Partials         65         41    -24

Powered by Codecov. Last update 84cfce3...7b84ac8

aslakknutsen · 2016-09-20T01:57:20Z

models/errors_whitebox_test.go

+	t.Parallel()
+	resource.Require(t, resource.UnitTest)
+	e := NotFoundError{entity: "foo", ID: "bar"}
+	assert.Equal(t, fmt.Sprintf(stNotFoundErrorMsg, e.entity, e.ID), e.Error())


Not quite sure what you're testing here...

@aslakknutsen, I test the Error() method of the NotFoundError struct.

Those tests seemed like low handing fruits.

I understand that part. But what your verifying is the formatting of the error string message. We have no strict requirement around these messages at this time and they are highly subject to rapid change. Adding test code to this part is only asking for test failure on any minuscule code change moving forward. If you wanted to 'touch' the errors to get coverage, a better strategy would be to test the scenarios/paths where they are used. That probably has more meaning.

I test the format, yes. But I'm also testing the errors as a unit. And even if we don't have a strict format right now. We might have it in the future and then we have a test for this. I strongly believe in what you said about scenarios and paths. But then I'm doing an integration test. So far I just wanted to test the errors unit. We can still layer tests that go through different packages as we go.

error is not a unit in my world. It has no logic.

Sure it does. It's a struct with functions on it. But no matter what the opinion is, the coverage output sees logic in there. Let's see if we run into troubles when updating the errors and talk about the concerns then, okay?

The concern is already present: The addition of random tests that is purely there to satisfy a coverage report with little real value to the code base and only add a maintenance overhead.

@aslakknutsen Is this your concern for the rest of this PR? Then I have wasted days basically.

I think in general what we should test is the intent of our code. The exact text of the error messages is not something the world knows or cares about. Therefore it can change without breaking the program. A meaningful test here would be that we we get back a message of >0 length and not nil, that the code doesn't crash for expected inputs, etc.

tsmaeder

I have some comments/suggestions, but I can live with what's there.

tsmaeder · 2016-09-22T13:11:57Z

models/equaler.go

+// DummyEqualer implements the Equaler interface and can be used by tests.
+// Other than that it has not meaning.
+type DummyEqualer struct {


I really don't understand what we need this struct for.

Seems it's only used for testing, if so, could we move it to one of the "_test.go" files?

It is used in all the tests from different packages to test the type difference of two Equaler implementations. See here for example.

tsmaeder · 2016-09-23T11:24:27Z

models/errors.go

@@ -2,6 +2,11 @@ package models

 import "fmt"

+const(
+	stBadParameterErrorMsg = "Bad value for parameter '%s': '%v'"


Why make a constant for sth that is used exactly once?

It is used in the error test to ensure the message is correct. This is what @aslakknutsen complained about because it adds maintenance overhead. My hope is that it pays off in the long run.

tsmaeder · 2016-09-23T11:28:36Z

models/field_definition_blackbox_test.go

 )

 func TestListFieldDefMarshalling(t *testing.T) {
+	t.Parallel()
+	resource.Require(t, resource.UnitTest)


I'm really unclear on what the concept behind "require" should be. "Database" is a resource we need to execute certain tests. But "UnitTest" is not a resource that can be present or absent. It's more of a label. Is there ever a reason do skip this test? I don't think so.

So the whole idea was to decide at runtime if a test shall be executed, remember? I know that is a bit strange that a Unit test is a resource. In fact if you look at the Require() implementation you can already see that UnitTest is treated in a special way. But it is not subject of this PR to discuss this implementation. Feel free to open another PR.

I've given some thought to the resource concept. I think we would be better off if we'd call it "precondition". Then resource.Database would become precondition.HAVE_DATABASE and resource.UnitTest would become precondition.RunUnitTest. The resource.Require() function could be renamed to precondition.IsFulfilled().

In total this means:

precondition.IsFulfilled(t, precondition.RunUnitTest)

Does this sound better @tsmaeder ? Feel free to create a PR an reference this discussion (#240 (comment)).

tsmaeder · 2016-09-23T11:30:20Z

models/fieldtype_blackbox_test.go

-	stDuration = SimpleType{KindDuration}
-	stURL      = SimpleType{KindURL}
-	stList     = SimpleType{KindList}
+	stString   = SimpleType{Kind: KindString}


Is this for consistency reasons or are you saying "SimpleType{KindString}" is bad?

To be honest: I'm not sure. Most of the time to be explicit is a good thing in Go. That's probably why I've written it out.

tsmaeder · 2016-09-23T11:32:43Z

models/list_type.go

@@ -46,12 +46,17 @@ func (fieldType ListType) ConvertFromModel(value interface{}) (interface{}, erro

 type converter func(FieldType, interface{}) (interface{}, error)

+const (
+	stErrorNotArrayOrSlice = "value %v should be array/slice, but is %s"


Again, I don't think these should be constants if used only once.

See my comment above.

tsmaeder · 2016-09-23T11:37:55Z

models/list_type_blackbox_test.go

+		SimpleType:    SimpleType{Kind: KindList},
+		ComponentType: SimpleType{Kind: KindInteger},
+	}
+	assert.False(t, a.Equal(c))


There should be a case where the two objects are equal:
assert.True(t, a.Equal(c))
assert.True(t, c.Equal(a)) // test for inverse

Good point.

See 7b84ac8

tsmaeder · 2016-09-23T11:47:50Z

models/errors_whitebox_test.go

+	t.Parallel()
+	resource.Require(t, resource.UnitTest)
+	e := NotFoundError{entity: "foo", ID: "bar"}
+	assert.Equal(t, fmt.Sprintf(stNotFoundErrorMsg, e.entity, e.ID), e.Error())


I think in general what we should test is the intent of our code. The exact text of the error messages is not something the world knows or cares about. Therefore it can change without breaking the program. A meaningful test here would be that we we get back a message of >0 length and not nil, that the code doesn't crash for expected inputs, etc.

This follows steps 1 and 2 of the article: "5 simple tips and tricks for writing unit tests in #golang" which can be found here: https://medium.com/@matryer/5-simple-tips-and-tricks-for-writing-unit-tests-in-golang-619653f90742 Shameless quote from the article above to justify this change: > 1. Put your tests in a different package > Go insists that files in the same folder belong to the same package, > that is except for `_test.go` files. Moving your test code out of the > package allows you to write tests as though you were a real user of the > package. You cannot fiddle around with the internals, instead you focus > on the exposed interface and are always thinking about any noise that > you might be adding to your API. > > This frees you up to change the internals however you like without > having to adjust your test code. > > 2. Internal tests go in a different file > If you do need to unit test some internals, create another file with > `_internal_test.go` as the suffix. Internal tests will necessarily be > more brittle than your interface tests — but they’re a great way to > ensure internal components are behaving, and are especially useful if > you do test-driven development.

Also: * removed build tags * properly renamed test files to blackbox and whitebox tests

Some tests were failing due to wrong whitebox assumptions. I'v re-written the initialization of unexported struct members from the package under test to use a New* ctor method.

kwk added the 💣 test label Sep 12, 2016

kwk self-assigned this Sep 12, 2016

aslakknutsen reviewed Sep 20, 2016

View reviewed changes

kwk force-pushed the kwk-improve-coverage branch from 44af5a7 to d195513 Compare September 20, 2016 07:35

tsmaeder approved these changes Sep 23, 2016

View reviewed changes

kwk changed the title ~~WIP Improve coverage of WorkItemType and WorkItem~~ Improve coverage of WorkItemType and WorkItem Sep 23, 2016

kwk added 21 commits September 23, 2016 15:19

Add Dummy Equaler

fc1b058

Improve coverage of WorkItemType.Equal()

d6679be

Improve coverage of WorkItem.Equal()

3f105a4

Improve coverage of models/errors.go

a013d60

Rename Test functions to properly state what get's tested

6dbb74a

Improve coverage of parseInts, parseLimit and NewWorkItemController

85aae8f

Fix comment

f57c78b

Improve coverage of Lifecycle.Equal

2924803

Improve coverage of version.go

981b048

Improve coverage of workitemtype_repository.go

2ef9265

Improve coverage of workitemtype_repository.go

b03069f

Improve coverage of models.EnumType.Equal()

8ebe831

Improve coverage of enum_type.go's contains() function

b1b7ba3

Introduce error template strings and re-use in test

8f07aa7

Introduce t.Parallel() for some tests

b3219b8

Also: * removed build tags * properly renamed test files to blackbox and whitebox tests

Fix warning: composite literal uses unkeyed fields

15ffb17

Fix unit tests

be2d6fe

Some tests were failing due to wrong whitebox assumptions. I'v re-written the initialization of unexported struct members from the package under test to use a New* ctor method.

Fix warning: composite literal uses unkeyed fields

86ddc16

Update documentation on resource package

db087da

Add models/list_type_blackbox_test.go

302f8c0

kwk added 8 commits September 23, 2016 15:19

Fix exported string from resource/resource.go

aa4ef4d

Added whitebox test for list_type.go

306cf88

Fix panic test for NewWorkitemController

150a4a9

Place TestMain() in whitebox test to have the correct package name

84d7320

Began writing models/simple_type_blackbox_test.go

a3cfac8

Test DummyEqualer

cd8c400

Use dot import for some blackbox tests

7ab454f

Fixup after rebasing on master

1358a50

kwk force-pushed the kwk-improve-coverage branch from 629202a to 1358a50 Compare September 23, 2016 13:20

kwk added 2 commits September 23, 2016 15:23

Test for equality in list_type_blackbox_test.go

7b84ac8

Fix merge issue

18d511a

kwk merged commit 1efe0b9 into master Sep 23, 2016

kwk deleted the kwk-improve-coverage branch September 23, 2016 15:39

Improve coverage of WorkItemType and WorkItem #240

Improve coverage of WorkItemType and WorkItem #240

kwk commented Sep 12, 2016 •

edited

kwk commented Sep 13, 2016 •

edited

kwk commented Sep 15, 2016

codecov-io commented Sep 16, 2016 •

edited

aslakknutsen Sep 20, 2016

kwk Sep 20, 2016 •

edited

aslakknutsen Sep 20, 2016

kwk Sep 20, 2016

aslakknutsen Sep 20, 2016

kwk Sep 20, 2016

aslakknutsen Sep 20, 2016

kwk Sep 23, 2016

tsmaeder Sep 23, 2016

tsmaeder left a comment

tsmaeder Sep 22, 2016

tsmaeder Sep 23, 2016

kwk Sep 23, 2016

tsmaeder Sep 23, 2016

kwk Sep 23, 2016

tsmaeder Sep 23, 2016

kwk Sep 23, 2016

kwk Sep 24, 2016

tsmaeder Sep 23, 2016

kwk Sep 23, 2016

tsmaeder Sep 23, 2016

kwk Sep 23, 2016

tsmaeder Sep 23, 2016

kwk Sep 23, 2016

kwk Sep 23, 2016

tsmaeder Sep 23, 2016

Improve coverage of WorkItemType and WorkItem #240

Improve coverage of WorkItemType and WorkItem #240

Conversation

kwk commented Sep 12, 2016 • edited

kwk commented Sep 13, 2016 • edited

kwk commented Sep 15, 2016

codecov-io commented Sep 16, 2016 • edited

Current coverage is 47.67% (diff: 100%)

Choose a reason for hiding this comment

kwk Sep 20, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder 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

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

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

kwk commented Sep 12, 2016 •

edited

kwk commented Sep 13, 2016 •

edited

codecov-io commented Sep 16, 2016 •

edited

kwk Sep 20, 2016 •

edited