Closed schmie closed 9 years ago
Comment by hywel on 6 Sep 2011 13:47 UTC Design:
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.
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.
Comment by hywel on 14 Sep 2011 12:58 UTC Patch attached. This includes the following:
Comment by rupert on 20 Sep 2011 17:41 UTC Minor points:
More to follow- I've yet to look at the tests themselves
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.
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"
Modified by hywel on 21 Sep 2011 20:32 UTC
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]),
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.
Comment by hywel on 22 Sep 2011 14:39 UTC
Merged into the latest push - this was non-trivial but the unit tests and regression tests still passed afterwards.
Pushed as 0c39180ac0ef.
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.