[Opensim-dev] Unit tests should not be coupled (WAS: Re: [Opensim-commits] r8737 - trunk/OpenSim/Framework/Communications/Tests)

[Mike Mazur]

Hi,

On Tue, 10 Mar 2009 17:03:44 +0900
Mike Mazur <mmazur at gmail.com> wrote:

> I'm pretty sure by using
> fixtures properly in OpenSim's unit tests, the need for Chained Tests
> can be eliminated.

I took a look at OpenSim/Data/Tests/BasicGridTest.cs. There are a few
tests there that depend on the order they're executed in order to
succeed. By removing the T???_ prefix from the methods, the unit tests
fail:

Test Case Failures:
1) OpenSim.Data.MySQL.Tests.MySQLGridTest.LoadEmpty :   Expected: null
  But was:  <OpenSim.Data.RegionProfileData>

at OpenSim.Data.Tests.BasicGridTest.LoadEmpty () [0x00000]
at (wrapper managed-to-native)
System.Reflection.MonoMethod:InternalInvoke
(object,object[],System.Exception&) at
System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags
invokeAttr, System.Reflection.Binder binder, System.Object[]
parameters, System.Globalization.CultureInfo culture) [0x00000]

2) OpenSim.Data.MySQL.Tests.MySQLGridTest.RegionList :   Expected: 2
  But was:  1

at OpenSim.Data.Tests.BasicGridTest.RegionList () [0x00000]
at (wrapper managed-to-native)
System.Reflection.MonoMethod:InternalInvoke
(object,object[],System.Exception&) at
System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags
invokeAttr, System.Reflection.Binder binder, System.Object[]
parameters, System.Globalization.CultureInfo culture) [0x00000]

This is expected. So I refactored those tests a little in order to get
rid of this dependency. I'm attaching the result to this email; look at
this email in the archives[1] to download the attachment.

Here's a summary of the changes:

- SimpleAddRetrieveProfile() test can be removed, as
  AddRetrieveCompleteTest() tests the same code
- RandomName() looks like a utility function and doesn't have any
  asserts, so it doesn't need the [Test] attribute
- extracted the region adding code in AddRetrieveCompleteTest() into a
  method called createRegion(UUID, string) which creates a region in
  the database; this method is called from any test that needs a region
  to exist in the DB
- added a [TearDown] method, which is called after every test has been
  executed by the NUnit framework, which removes all the regions from
  the DB

If nobody has any strong objections to this approach to isolating
tests, I'll commit this tomorrow. Also, it's probably a good idea to
refactor other tests in a similar fashion. If chaining tests is not
absolutely necessary, it shouldn't be done.

One more thing to note, those tests use GridDataBase.AddProfile() to add
regions to the DB and GridDataBase.GetProfile*() to get the region back.
This is flawed; imagine the GridDataBase implementation had a bug that
prevented anything from actually being stored to disk. Its caching
mechanism worked as expected, though, and anything that was added with
AddProfile() was always available with GetProfile*() as long as the
application (grid server) was running. The functionality of actually
reading the disk/DB was never actually tested.

To ensure the correct functionality is being tested, one would insert
the data into the DB by different means (possibly SQL statements in the
tests), then retrieve them using the proper GetProfile*() method.

Thanks,
Mike


[1]
https://lists.berlios.de/pipermail/opensim-dev/2009-March/thread.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: BasicGridTest.cs
Type: text/x-csharp
Size: 10043 bytes
Desc: not available
URL: <http://opensimulator.org/pipermail/opensim-dev/attachments/20090310/ec2be39b/attachment-0001.bin>