293 test witrepos #294

tsmaeder · 2016-09-28T10:53:17Z

Issue #293

tsmaeder · 2016-09-28T13:56:18Z

The tests probably won't succed (becuase there is no migration code in this PR.).

bartoszmajsak · 2016-09-28T14:10:06Z

models/workitem_repository_blackbox_test.go

+)
+
+func TestCreateLoadDeleteListWI(t *testing.T) {
+	doWithTransaction(t, func(ts *models.GormTransactionSupport) {


How many things are we testing at once? Wouldn't it be better to split it to more focused, descriptive tests? This test itself has too many reasons to fail in my opinion.

Can you explain why that is a bad thing?

Hard to reason what the test is about

It tests too many things at once - as method already suggests.

If something stops working you will go through snow-ball trying to fix every single failing assertion one after the other

Is it good enough or still not convinced?

The idea that you just have to look at a test to know what is broken is a red herring in most cases. You'll have to reproduce and debug the code anyway. The target code of this test is colocated and highly coupled. I don't want to copy/paste the setup code x times just to satisfy an abstract notion of niceness. Our guidelines should favor easy & fast writing of tests.
What I'm saying is splitting this test method into 5 different ones will not increase coverage and it won't allow us to debug failures faster. At least I'm convinced of that.

The idea that you just have to look at a test to know what is broken is a red herring in most cases

Not from my experience.

I don't want to copy/paste the setup code x times just to satisfy an abstract notion of niceness.

Don't you have something like beforeEach or "extract to function" to easily fix it?

As I'm not really part of the core team I won't continue this discussion further, but I truly believe that this is not the way how the code should be written at all. It will get hard to maintain later to start with. And coverage should be just a supportive metric for your tests, not the goal.

A Test failure can be seen as a Bug Report, the more fine-grained/detailed the failure report is the easier it is to reason about and put you on the right track to fix it.

In the current state a test failure says; 'something wrong with WI'.

I guess that's a good read http://xunitpatterns.com/Test%20Smells.html

bartoszmajsak · 2016-09-28T14:12:36Z

models/main_blackbox_test.go

+		if err != nil {
+			panic("Failed to connect database: " + err.Error())
+		}
+		defer db.Close()


Shouldn't defer be before panic?

No, if opening the db fails, it's not open.

Is it the only reason to fail here?

bartoszmajsak · 2016-09-28T14:13:38Z

models/workitemtype_repository_blackbox_test.go

+	if err := ts.Begin(); err != nil {
+		panic(err.Error())
+	}
+	defer ts.Rollback()


Same as before, shouldn't defer be before panic?

Anyway what's the point of the transaction here if we anyway rollback before it even hits the database?

To leave no traces of the test in the database.

Then why do we need the database at all? Shouldn't we commit and clean-up for each and every test before we start?

We need the database because writing stuff to the database is what we're testing. Rolling back the transaction is the simplest way to write tests without having side effects in the db.

What is the transaction isolation level we are using?

What happens to the actual DB depends highly on the DB Driver. If you take Hibernate/JDBC as an example, create the object and fetch the object from PersistenceContext might never actually flush anything to the DB unless you commit. DB operation tests should always split Write and Read in two different TX (if TX is required at all) to ensure we actually hit the DB.

The idea that Rollback DB on test end actually work is bogus.

What you are saying is true in the case of Hibernate. Usually, you can force writes to the DB by calling some kind of flush() method, though.
The problem with committing the changes is that it makes side-effect free tests impossible: sooner or later you'll end up with spurious failures.

I see several advantages of not leaving a trace by doing a rollback

little to no need of cleanup before the test

ability to run the tests in parallel, especially those which share the schema

but I think both of these are still possible to address when doing a commit.

However, my main question is - does the thing we are testing here, with rollback (and maybe flushing) reflects what's going on on production? If not such a test is simply a waste, isn't it?

I wouldn't actually call it a waste...many unit tests have little to do with what happens in production (for example, because they mock certain systems away). That doesn't mean they are not testing anything. Same here...even while some things may go wrong at commit time a large number of codepaths are tested.
In the worst case that stuff would go wrong at commit time, you may get false negatives, but you won't get false positives, so if the test fails, you are certain that something is wrong. On the other hand, if the test succeeds, you are not sure it will not fail in production.

But anyway...no quite sure how we would implement idempotent tests while committing changes to the database.

codecov-io · 2016-11-02T09:47:57Z

Current coverage is 64.74% (diff: 100%)

Merging #294 into master will increase coverage by 0.48%

@@             master       #294   diff @@
==========================================
  Files            45         45          
  Lines          1936       1940     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1244       1256    +12   
+ Misses          560        554     -6   
+ Partials        132        130     -2

Powered by Codecov. Last update 8662307...85bbac5

hectorj2f · 2017-02-20T19:49:26Z

@bartoszmajsak @aslakknutsen Do we still need this ? what is its state ?

hectorj2f · 2017-07-19T15:45:41Z

Closing this PR! we don't need it anymore

bartoszmajsak reviewed Sep 28, 2016

View reviewed changes

tsmaeder force-pushed the 293_test_witrepos branch 2 times, most recently from b9de4d1 to c7f9ca7 Compare October 3, 2016 11:34

tsmaeder force-pushed the 293_test_witrepos branch from c7f9ca7 to 4df1cbb Compare November 1, 2016 15:29

tsmaeder force-pushed the 293_test_witrepos branch from 33a98f6 to 3ecc4b3 Compare November 2, 2016 10:01

tsmaeder added 4 commits November 7, 2016 15:09

Added tests for work item repositories

e8781f1

Fix errors found when adding tests

f15a13f

Don't rely on transaction behaviour for setup

f30eb35

Require database for test

85bbac5

tsmaeder force-pushed the 293_test_witrepos branch from 3ecc4b3 to 85bbac5 Compare November 7, 2016 14:36

hectorj2f closed this Jul 19, 2017

293 test witrepos #294

293 test witrepos #294

tsmaeder commented Sep 28, 2016

tsmaeder commented Sep 28, 2016

bartoszmajsak Sep 28, 2016

tsmaeder Sep 30, 2016

bartoszmajsak Sep 30, 2016

tsmaeder Oct 3, 2016

bartoszmajsak Oct 3, 2016 •

edited

aslakknutsen Oct 3, 2016 •

edited

bartoszmajsak Oct 4, 2016

bartoszmajsak Sep 28, 2016

tsmaeder Sep 30, 2016

bartoszmajsak Sep 30, 2016

bartoszmajsak Sep 28, 2016

bartoszmajsak Sep 28, 2016

tsmaeder Sep 30, 2016 •

edited

bartoszmajsak Sep 30, 2016 •

edited

tsmaeder Oct 3, 2016 •

edited

bartoszmajsak Oct 3, 2016

aslakknutsen Oct 3, 2016

tsmaeder Oct 3, 2016

bartoszmajsak Oct 3, 2016

tsmaeder Oct 4, 2016

codecov-io commented Nov 2, 2016 •

edited

hectorj2f commented Feb 20, 2017

hectorj2f commented Jul 19, 2017

293 test witrepos #294

293 test witrepos #294

Conversation

tsmaeder commented Sep 28, 2016

tsmaeder commented Sep 28, 2016

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

bartoszmajsak Oct 3, 2016 • edited

Choose a reason for hiding this comment

aslakknutsen Oct 3, 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

tsmaeder Sep 30, 2016 • edited

Choose a reason for hiding this comment

bartoszmajsak Sep 30, 2016 • edited

Choose a reason for hiding this comment

tsmaeder Oct 3, 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

codecov-io commented Nov 2, 2016 • edited

Current coverage is 64.74% (diff: 100%)

hectorj2f commented Feb 20, 2017

hectorj2f commented Jul 19, 2017

bartoszmajsak Oct 3, 2016 •

edited

aslakknutsen Oct 3, 2016 •

edited

tsmaeder Sep 30, 2016 •

edited

bartoszmajsak Sep 30, 2016 •

edited

tsmaeder Oct 3, 2016 •

edited

codecov-io commented Nov 2, 2016 •

edited