Re: [lmi] tests (was: C++11 transition...)

[Vadim Zeitlin]

On Tue, 17 May 2016 22:31:50 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-05-17 21:56, Vadim Zeitlin wrote:
GC> [...]
GC> >  As I wrote there, most of the work was done by clang-tidy and I just
GC> > reviewed its results and rebuilt (under all platforms, including using the
GC> > official makefiles) and reran the tests (the rounding one indeed fails, as
GC> > you recently mentioned, but no other ones seem to do, although I'm always
GC> > nervous about this because the test suite doesn't clearly state how many
GC> > tests have failed at the end, so it's easy to miss something, I'd like to
GC> > change this...).
GC> 
GC> I'm reluctant to touch another low-level part of lmi. There's no urgent
GC> need to change this.

 I'd really like to ask you to reconsider. IMO it's pretty important to
have a simple, automatic "check" (or "unit_tests" or whatever -- it's not
the name that matters) make target which can be used to unambiguously and
automatically check if the test suite passes or not. The absence of such
target in lmi makefiles is a complete mystery to me.

GC> I do
GC>   make $coefficiency unit_tests >../log 2>&1
GC>   sed -f diagnostics.sed ../log 2>&1 |less -S
GC> and then type
GC>   /\*\*
GC> in 'less'. I skip the six double asterisks that show problems in the
GC> std::round() function that we don't use in production, and I skip the
GC> intentional errors in the unit test of the unit-test system.

 This is too error-prone. Mistakes can be easily made in a process
involving non-trivial manual steps and this means they will be made. There
should really be just a single, easy to check, indicator at the end showing
whether the tests passed or not. And it's not like it's even difficult to
do, we could have something like this for example:

---------------------------------- >8 --------------------------------------
diff --git a/workhorse.make b/workhorse.make
index 1691a15..f4f19c3 100644
--- a/workhorse.make
+++ b/workhorse.make
@@ -1130,6 +1130,17 @@ run_unit_tests: unit_tests_not_built $(addsuffix 
-run,$(unit_test_targets))
        @$(ECHO) -e "\nRunning $*:"
        @-./$* --accept
 
+.PHONY: check
+check: build_unit_tests
+       @errors=0; \
+       $(foreach t,$(unit_test_targets),./$t --accept || errors=`expr $$errors 
+ 1`;) \
+       if [ $$errors -eq 0 ]; then \
+           echo "$(words $(unit_test_targets)) unit tests passed"; \
+       else \
+           echo "$$errors unit tests failed"; \
+       fi; \
+       exit $$errors
+
 
################################################################################
 
 # Test command-line interface.
---------------------------------- >8 --------------------------------------

It could be improved in many ways, of course, but just adding this target
will already be a big improvement IMO.

GC> Then there's timer_test.
GC> 
GC> Fixing the actual problems demonstrated by failing tests is far more
GC> valuable than summarizing them.

 Of course, but I'd say that the latter is a prerequisite of the former. If
you don't even notice that a test fails, you don't risk to fix it.

 This being said, should I look at the rounding test once again? I'd like
to see the test suite pass, of course, but I'm not sure if you're going to
trust my changes to this code...


GC> I keep meaning to look up a message you sent a while ago about
GC> 'timer_test'; I think you pointed me to a wx unit test, but at this
GC> rate I won't have time to look at that this year, so might I ask you to
GC> connect the dots for me and propose a concrete change?

 I think this was already addressed by r6602, but please let me know if I'm
missing anything.

GC> Could we take a time-out to let me
GC>  - finish my pending changes, and
GC>  - expunge RCS '$Id$' globally, touching pretty much every file,
GC>    as the first step toward migrating from svn to git
GC> and then turn the PR spigot back on?

 Sure, please let me know when you're done so that I could resume
submitting the next ones.

 Thanks in advance,
VZ