Add unit-testing for the lattice-Boltzmann kernel

[schmie]

Reported by hywel on 6 Sep 2011 13:09 UTC Add unit-testing to HemeLB. This ticket will be concerned with only the lattice-Boltzmann kernel. Further tests will be added on later issues.

[schmie]

Comment by hywel on 6 Sep 2011 13:47 UTC Design:

• I'm going to use CppUnit (https://launchpad.net/cppunit2) for this, mainly because it's available as a package for Ubuntu (where CxxTest isn't). It also supports fixtures and uses macros for asserts in the same way CxxTest does.
• I'm going to create a new hemelb::tests namespace (and corresponding directory structure) within the Code folder of the repository.
• The only tests I'm going to add are for the LB kernel, so I'll have a Code/Tests/LBTests folder and namespace with one file and one class for each of the following categories: Kernel Tests, Collision tests and Streamer Tests. NB. this will have tests for (e.g.) all kernels in the code in the same file.
• These will test all public functions of the 2 kernels, 4 collision operators and 1 streaming operator implemented. I'll use the same, tiny, artificial geometry for each one, and work out the correct collided and streamed values by hand to compare against (where possible).
• I may also need to adapt the rest of the code to make the tests pass.
• I'll make it so the tests are not compiled in the HemeLB executable and are only compiled if a specific flag is set in the machine's make/config file.

[schmie]

Comment by miguel on 6 Sep 2011 16:24 UTC Your design sounds reasonable to me. My only comment is that it might be a bit confusing to have a hemelb/Tests and a hemelb/Code/Tests directories. One option is to unify them, another to use different names (i.e. !UnitTests, !RegresionTests).

I don't need to review the final design.

Just a pedantic note on style, trac considers anything written in camel case to be a link to a wikipage (i.e. CppUnit), if the wikipage does not exist it will show up as a broken link. You can escape it with !, !CppUnit.

[schmie]

Comment by hywel on 7 Sep 2011 12:35 UTC All good points. You're right about the test-naming, so I'll do that, and I'll try to remember to be careful when !CamelCasing in the future.

[schmie]

Comment by hywel on 14 Sep 2011 12:58 UTC Patch attached. This includes the following:

• Move the regression tests from /Tests to /RegressionTests
• Increase the amount of code re-use in D3Q15
• Use distribn_t in D3Q15 rather than double
• Add a separate build output for the unit tests (which are not included in the main HemeLB build)
• Some code-tidying
• Encapsulate the LBM Parameters better (it's now responsible for recalculating its own tau, omega etc). Not sure if voxelSize is the optimal name for that parameter though.
• Fix a bug I found with the BoundaryComms object which occurred when only using one core (e.g. in the unit-tests)
• Added test versions for the LatticeData and SimConfig objects.
• Added tests for all the kernel, collision and streamer implementations currently available. These rely on helper functions quite a bit.

[schmie]

Comment by rupert on 20 Sep 2011 17:41 UTC Minor points:

• are the changes to the Eclipse project deliberate?
• Can you put a note somewhere about how to run the unit tests and what software is needed? (I don't want to have to think)
• Naming of class hemelb::unittests::!TestLatticeData - slightly uniformative name maybe hemelb::unittests::!FourCubeLatticeData is better or something like that? (Test-ness is indicated by namespace) Similarly for !TestSimConfig.
• In hemelb::unittests::lbtests::CollisionTests::setUp there's not a single comment!

More to follow- I've yet to look at the tests themselves

[schmie]

Comment by rupert on 21 Sep 2011 16:58 UTC I tried to apply the patch to my tree (from rev 3ca8cd992101) but it fails with:

julian:hemelb rupert$ patch -p1 < ~/Downloads/unitTesting.diff 
patching file  XXXXXXX <snip>
patching file Code/lb/kernels/Entropic.h
patch: **** malformed patch at line 586: @@ -107,8 +110,7 @@

So I can't test your tests! Can you attach a working patch file or send me the path to your repo and a revision hash to look at please?

Cheers.

[schmie]

Comment by hywel on 21 Sep 2011 20:31 UTC Eclipse project: No they weren't, it'll just be some options. I think it was to remove certain comment-keywords that cause things to appear in the 'Tasks' window. We still get all the TODOs and FIXMEs.

Unit tests note: see Unit tests. Just added.

Renaming: done.

Commenting: done.

I've incorporated these changes into the new patch. Not sure what went wrong last time but I've checked this works for me. "hg update 3ca8cd992101 && hg patch unitTestDiff2.diff"

[schmie]

Modified by hywel on 21 Sep 2011 20:32 UTC

[schmie]

Comment by rupert on 22 Sep 2011 09:28 UTC In changeset 09b8ae313e74 you introduced a build error: Code/steering/basic/HttpPost.cc:125: error: ip_addr was not declared in this scope. The variable ip_addr is declared only within the for loop, but used after it---this should never build! Removing this part of the changeset will fix the problem

diff -r 6db8c40bd1f9 -r 09b8ae313e74 Code/steering/basic/HttpPost.cc
--- a/Code/steering/basic/HttpPost.cc   Fri Sep 02 14:28:05 2011 +0100
+++ b/Code/steering/basic/HttpPost.cc   Fri Sep 02 15:25:59 2011 +0100
@@ -32,7 +32,6 @@
       // This really does modify the strings passed in. Without
       // checking their lengths, of course.
       char hostname[     char ip_addr[16](256];
-);

       // On specific machines, just get the host name and insert it into the rank_0_host_details parameter
 #ifdef NGS2Leeds
@@ -80,6 +79,8 @@
           continue;
         }

+        char ip_addr[        sprintf(ip_addr,
                 "%d.%d.%d.%d",
                 (unsigned char) (host->h_addr_list[address_id](16];
+
)[0]),

[schmie]

Comment by rupert on 22 Sep 2011 09:51 UTC Unit tests: These build and pass now! Good job.

HOWEVER: I'm not sure that the goals of these tests are. Surely this is important. Some are obvious to me (eg StreamerTests::TestSimpleCollideAndStream) but others are opaque (e.g. CollisionTests and KernelTests)

Once the purposes of these tests are described, I'm happy with this; no need to further review.

[schmie]

Comment by hywel on 22 Sep 2011 14:39 UTC

• Fixed that bug.
• Renamed the kernel test methods to be "TestCalculationsAndCollision"
• Add comments to both kernel tests explaining the two cases tested by each function.
• Added comments to top of collision tests explaining the premise.
• Added comments to top of streamer tests explaining the premise.

Merged into the latest push - this was non-trivial but the unit tests and regression tests still passed afterwards.

Pushed as 0c39180ac0ef.