Ayende @ Rahien

Refunds available at head office

A horrible, HORRIBLE, horrible test

I think, without question, that this is one of the most horrible nasty tests that I have seen. It has been ported as is from the java code, and you can literally feel the nastiness in reading it:

[Test]
public void IndexedEmbeddedAndCollections()
{
	Author a = new Author();
	a.Name = "Voltaire";
	Author a2 = new Author();
	a2.Name = "Victor Hugo";
	Author a3 = new Author();
	a3.Name = "Moliere";
	Author a4 = new Author();
	a4.Name = "Proust";

	Order o = new Order();
	o.OrderNumber = "ACVBNM";

	Order o2 = new Order();
	o2.OrderNumber = "ZERTYD";

	Product p1 = new Product();
	p1.Name = "Candide";
	p1.Authors.Add(a);
	p1.Authors.Add(a2); //be creative

	Product p2 = new Product();
	p2.Name = "Le malade imaginaire";
	p2.Authors.Add(a3);
	p2.Orders.Add("Emmanuel", o);
	p2.Orders.Add("Gavin", o2);


	ISession s = OpenSession();
	ITransaction tx = s.BeginTransaction();
	s.Persist(a);
	s.Persist(a2);
	s.Persist(a3);
	s.Persist(a4);
	s.Persist(o);
	s.Persist(o2);
	s.Persist(p1);
	s.Persist(p2);
	tx.Commit();

	s.Clear();

	IFullTextSession session = Search.CreateFullTextSession( s );
	tx = session.BeginTransaction();

	QueryParser parser = new MultiFieldQueryParser( new string[] { "name", "authors.name" }, new StandardAnalyzer() );

	Lucene.Net.Search.Query query = parser.Parse( "Hugo" );
	IList result = session.CreateFullTextQuery( query ).List();
	Assert.AreEqual( 1, result.Count, "collection of embedded ignored" );

	//update the collection
	Product p = (Product) result[0];
	p.Authors.Add( a4 );

	//PhraseQuery
	query = new TermQuery( new Term( "orders.orderNumber", "ZERTYD" ) );
	result = session.CreateFullTextQuery( query).List();
	Assert.AreEqual( 1, result.Count, "collection of untokenized ignored" );
	query = new TermQuery( new Term( "orders.orderNumber", "ACVBNM" ) );
	result = session.CreateFullTextQuery( query).List();
	Assert.AreEqual( 1, result.Count, "collection of untokenized ignored" );

	tx.Commit();

	s.Clear();

	tx = s.BeginTransaction();
	session = Search.CreateFullTextSession( s );
	query = parser.Parse( "Proust" );
	result = session.CreateFullTextQuery( query ).List();
	//HSEARCH-56
	Assert.AreEqual( 1, result.Count, "update of collection of embedded ignored" );

	// Tidy up
	s.Delete(a);
	s.Delete(a2);
	s.Delete(a3);
	s.Delete(a4);
	s.Delete(o);
	s.Delete(o2);
	s.Delete(p1);
	s.Delete(p2);
	tx.Commit();

	s.Close();
}

Point to anyone who want to start a list of how many good test metrics this test violates.

Oh, and right now, this test fails.

Comments

Lee Brandt
04/20/2009 05:29 PM by
Lee Brandt

Wow... let me tell you.. .wow.

My head is kinda spinning... I don't know exactly how many things are actually happening in this test.

wow...

Yann Schwartz
04/20/2009 05:29 PM by
Yann Schwartz

The real WTF (tm) is that the person who wrote the test wrote :

o2.OrderNumber = "ZERTYD";

which is clearly something you get when typing a pseudo random string on a AZERTY (french) keyboard, but at the same time wrote

o.OrderNumber = "ACVBNM";

which is more a QWERTY kind of pseudo-randomness.

This test was written by a schizophrenic.

Timacho
04/20/2009 05:41 PM by
Timacho

Not that I write tests like that. But everyone's comments and the OP are invalid and mean nothing unless accompanied by how it should be written. All I hear is bitching, nothing useful.

Flaker
04/20/2009 06:01 PM by
Flaker

Without counting the amazing amount of lines per method and repeated code? ..Did you change the variable names? (a3, o) or those are the originals?

Andrew
04/20/2009 06:07 PM by
Andrew

Timacho, the next post shows a refactored test...

Peter Morris
04/20/2009 06:19 PM by
Peter Morris

I got bored of reading it. I did laugh though when I read "Oh, and right now, this test fails."

Diego Mijelshon
04/20/2009 06:39 PM by
Diego Mijelshon

Besides the details some people mentioned, the most important problem with that test is that IT'S IMPOSSIBLE TO SEE WHAT IT'S TRYING TO TEST EXACTLY.

A test should check ONE ASPECT of ONE COMPONENT. Otherwise, it's impossible to tell which part of which component is not working (or is not implemented yet)

IF you really, really need to test several objects at the same time, at least you set them up separately, and then have as many methods as assertions there are.

I could go on, but it's pointless.

jdn
04/20/2009 07:16 PM by
jdn

I hate it when my untokenized collections are ignored.

James Carr
04/20/2009 07:57 PM by
James Carr

Bah that's nothing... i see those kinds of tests in legacy code all the time. :)

Jonathan Allen
04/20/2009 08:03 PM by
Jonathan Allen

I see nothing wrong with that. You can't just test each trivial piece in isolation, sometimes you have to put the whole thing together and test it like you would actually use it.

Actually I find the test quite revealing. Not only is it a decent integration test, it demonstrates some deficiencies in the design of the underlying API.

As for the test failing, so what? Unless your test framework is completely lame, it will let you step through the test to see exactly where the failure occurred.

Jason
04/20/2009 08:14 PM by
Jason

I would not waste any time naming what's wrong with it beyond lack of documentation... assuming that either the original author is around to document it, or I don't need this piece of code to continue my work.

roger
04/20/2009 08:53 PM by
roger

Ok, this test is ugly.

First of all - if database is shared between many developers this test passes or fails depending on race conditions between people running it.

Also - when it fails after first commit but before last, it leaves db in wrong state and it won't pass next time. We have plenty of such tests in our app and I know symptoms :)

But - it still is better than no test at all. It is also faster to write one test that checks many things at once, than to write many simple and elegant unit tests. When anything goes wrong you can see in stack trace what exactly is wrong, so this kind of test gives you many of the benefits of unit tests done right, while it is easier to write.

Of course it would be better to write it the Right Way (tm). But it will take more time and IMHO sometimes it's not worth it.

Ok, now I wait for all the bashing of TDD activists :)

Andrew
04/20/2009 09:09 PM by
Andrew

The problem there Roger is you start hitting that slippery slope, at what point do you stop combining tests?

I'm not a huge TDD guy, but we have a project here that thankfully I'm not working on that instead of individual unit tests, they felt it would be more "realistic" to combine tests and setup/teardown the database between running the tests. Of course, now it takes six hours (yes, six hours) to do a test on a system which isn't even that large, 98% of which is database related.

The best part is they want to set it up to do CI...imagine waiting six hours every check in? ;)

roger
04/20/2009 09:30 PM by
roger

We have simple rule - you can only change test when you change functionality it tests.

When you add new functionality you have to add new test.

So it's up to original author to define scope of test, later it won't grow. And as original author is probably lazy, test will probably only test what it really have to :)

So for now our tests are like 10-100 lines of code. Most of them below 20 LOC.

What I don't like is that such tests mess with database state. We have shared database with all developers and testers, and tests are run automatically on every compile of every project by maven, so when 2 people compiles one project at once, and tests happens to lock the same row in some table, one of them fails and lefts mess. Then we have to manually deleted this mess. This is bad and we work on that, but at least when something don't work everybody knows at least an hour later. And everybody knows which commit it was.

As for time - each project builds in 5 minutes or less, this is because we don't set up db in our tests.

Personally I'd prefer to have separate db on every computer, other than that I'm pretty happy with our setup.

Jonathan Allen
04/20/2009 09:55 PM by
Jonathan Allen

@roger

Sounds like you have two serious issues to address.

First, get yourself some more databases. There is no excuse for sharing one database across all developers, especially with today's hardware prices.

Secondly, why the hell are you running your integration tests with every compile? Hell, you shouldn't even be running all of your unit tests with every compile.

Jonathan Allen
04/20/2009 09:59 PM by
Jonathan Allen

@Andrew

Seems to me that they drank the unit testing kool-aid. There is nothing wrong with a 6 hour integration test, hell I used to oversee 24-hour long stress tests. But to treat them as part of the check-in process seems downright stupid.

Andrew
04/20/2009 10:27 PM by
Andrew

Jonathan,

I believe our build engineer's threat to go on a killing spree solved the "can we do this on every check in, please?" issue. ;)

roger
04/20/2009 10:42 PM by
roger

@Jonathan Allen

We don't run all tests with every compile - only tests for the project you are compiling (we have more than 20 projects that make one app). So typically compile is 3-5 minutes.

Still too long, but not that bad.

As for database - I think it's because we have Oracle (we need it because legacy app we interface with uses stored procedures), and Oracle on every developers' computer will be hard to maintain. But that's not my decision, and in fact I advocated for using separate databases.

Ayende Rahien
04/21/2009 04:44 AM by
Ayende Rahien

Roger,

There is no original author for this.

And unreadable test means that when it is failing, it is REALLY hard to understand what is going on there.

Ayende Rahien
04/21/2009 04:45 AM by
Ayende Rahien

Roger,

a) Oracle Express

b) Create different databases on the same server for each dev

c) Create different schema on the same database for each dev.

mike
04/21/2009 07:30 AM by
mike

Maybe they should have put down the Voltaire and tried to be more optimistic about writing a good test? :)

critic
04/21/2009 07:54 AM by
critic

So how should this test be written then ??

Bashing someone's stuff is too easy

Ayende Rahien
04/21/2009 07:56 AM by
Ayende Rahien

Critic,

Did you see the next post?

bob
04/21/2009 09:36 AM by
bob

but typical

ken
04/28/2009 12:14 AM by
ken

So please elaborate on how one is supposed to know that this "next post" is even relevant to this post? I see no links or mention of it in the original article. The very unhelpful menu at the top of the post makes no connection between them either -- no mention of a "part 2", etc.

I also don't see a chronological listing of recent posts anywhere, such that when I finish this one I look over and see this mysterious follow-up everyone is alluding to. I mean, do you really need access to each month of the past 5 years on the sidebar? I dare you to do a usability test; I'll bet you $5 that those links are used by about 5% of your visitors (and do they use them alot? Or are they just curious and clicked once for the hell of it?). How to fix? Just show all your posts from the past month (or 3 months, whatever) and give a link for the rest of the archives. You may be a good programmer (or you may not, since this is the only post that I have for reference), but this site's UI is horrendous.

Ayende Rahien
04/28/2009 04:41 AM by
Ayende Rahien

Ken,

Do you see the next and prev posts on the top of this post?

You are aware that this is a blog? A chronological listing of posts?

As for the rest, I use the archives, quite a lot, frankly.

Comments have been closed on this topic.