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

[Arthur Valadares]

I think in there lies the confusion, which Stefan mentioned. Our unit
tests are doing more then testing the object itself. The tests in the
Data directory are testing data persistence on the database and not just
the object. Although this might not be considered a unit test (as
Stephan mentioned), it is actually a needed and important test to make
sure we aren't breaking the plugin and causing horrible damages to
existing databases. 

So if you're going to test if data is persisting on the database, you
must first insert the data to be able to update it. 

Not sure I got this right, but this is what I understood from your
e-mail. If this is the case, maybe Stefan is right and the tests should
be divided in unit and integration tests, where a refactoring would be
needed. 

On Wed, 2009-03-11 at 09:31 +0900, Mike Mazur wrote:
> Hi,
> 
> On Tue, 10 Mar 2009 17:10:42 -0300
> Arthur Valadares <arthursv at linux.vnet.ibm.com> wrote:
> 
> > Still some tests must be chained (you can't test an Update without a
> > Create for example). And for that reason, and also to keep the logic
> > of whoever reads the test, I feel we should still keep writing tests
> > with some kind of logical order. 
> 
> An update can definitely be tested without testing create first. To test
> the update, the data would be inserted into the DB by another means
> first. For example:
> 
> // pseudocode
> [TestFixture]
> public class MyTest
> {
>   UUID m_regionUUID = <some UUID>;
> 
>   [SetUp]
>   public void SetUp()
>   {
>     CreateNewDB("OpenSim");
>     sql.execute("INSERT INTO regions VALUES('My Region',m_regionUUID)");
>   }
> 
>   [TearDown]
>   public void TearDown()
>   {
>     DropDB("OpenSim");
>   }
> 
>   [Test]
>   public void TestCreate()
>   {
>     UUID thisUUID = <another UUID>;
> 
>     object_under_test.createRegion("New Region", thisUUID)
> 
>     Region region = sql.execute("SELECT FROM regions WHERE
>     uuid=thisUUID");
>     assertEquals(region.UUID, thisUUID);
>     assertEquals(region.Name, "New Region");
>   }
> 
>   [Test]
>   public void TestUpdate()
>   {
>     Region updatedRegion = new Region();
>     updatedRegion.UUID = m_regionUUID;
>     updatedRegion.Name = "Updated region";
> 
>     object_under_test.updateRegion(m_regionUUID, updatedRegion);
> 
>     Region region = sql.execute("SELECT FROM regions WHERE
>     uuid=m_regionUUID");
>     assertEquals(region.UUID, m_regionUUID);
>     assertEquals(region.Name, "Updated Region");
>   }
> }
> 
> I'm actually trying to show two things here:
> 
> 1. The SetUp and TearDown methods are called by the NUnit framework
>    before and after each test is executed, repsectively. They ensure
>    that each and every single test starts with the exact same data
>    populated in the DB. This means each test can be run independently
>    of any other test and doesn't depend on any other test being
>    run/succeeding/failing.
> 
> 2. The object under test is only used in the tests. In the SetUp,
>    TearDown, and even verification code, another (known to work) method
>    is used to interact with the database (in the example, it's raw
>    SQL). In BasicGridTest.cs I noticed that the object under test
>    (the GridDataBase class) is being tested, but it's also used to
>    populate the database, and to verify that the data was indeed
>    written to the database/disk. This is not good, as I mentioned in
>    another email[1].
> 
> > This will also help debugging some unit tests where you modify
> > something but has no knowledge that this modification changes your
> > "expected initial status" of other tests. There are lot's of obscure
> > relations in OpenSim and a test creator might fail to see the whole
> > picture.
> 
> If a developer makes a change that affects "expected initial status" of
> many other tests, this is a warning sign that the developer is changing
> something of importance. If this change is indeed necessary, then all
> the tests that are affected should be modified to reflect the new state
> of the source code. If fixtures (SetUp, TearDown) are used properly,
> the changes will likely be limited to them.
> 
> > In summary, I don't see a reason not to organize the tests with TXXX.
> > I'll keep working them that way, but this is another one of those
> > discussions with no right and wrong, just hunches of what's more
> > efficient. :)
> 
> Sure.
> 
> Mike
> 
> 
> [1]
> https://lists.berlios.de/pipermail/opensim-dev/2009-March/005625.html