Tcl Source Code

With tcltest-1.0, if I called the tcltest::test proc 
with the wrong number of arguments, or a wrong 
argument, it threw an error.  With tcltest-2.0.1, it 
returns 1.

This is a significant regression.  It is not 
reasonable to fill up my .test files with checks for 
the return value of tcltest::test.  Yet, if I omit 
such checks, the failure would be easy to overlook.

This patch replaces every 'return 1' with a call 
to 'error'.

Logged In: YES 
user_id=80530

committed to HEAD (8.4a5)

File Added - 21063: patch-2002-04-12

Logged In: YES 
user_id=80530

latest patch includes documentation updates.

File Added - 21020: patch-2002-04-11

Logged In: YES 
user_id=246646

Ah, convergence!  I like that patch, especially the "Side
effects" comment.  This new one (patch-2002-04-11) is
identical as far as tcltest.tcl goes and I fixed the rest of
the tests.  Also, looking at your patch pointed out that the
[llength $args]==0 case -- which I had broken and you fixed
-- was not tested.  I added one new test for that
(tcltest-21.0).

File Added - 20919: patch-2002-04-10

Logged In: YES 
user_id=80530

Thanks for updating the patch.  Much shorter now that
the whitespace mess is cleaned up!

There's no need to move to 2.0.3 with this patch.  No 2.0.2
release has been made yet, so more changes to it are OK.

I agree that the current code is neither fish nor fowl, and
needs changing.  It makes no sense that [test] and
[test foo] raise errors, but [test foo bar -badOption]
does not.  I tend to favor your request.  If we go that
way, we should go all the way, and return to having
[test] return an empty string on success.  Returning 0
doesn't really make sense unless it could also return 1
in some other circumstance.

Here's a revised patch.

File Added - 20897: patch.txt

Logged In: YES 
user_id=246646

I would like to lobby a second time for this patch.  I have
reconstructed Don's patch so that:

 1) it works on the latest CVS HEAD
 2) the tests pass
 3) the package version is incremented to 2.0.3

This patch implements a contract (as in "programming by
contract") between the caller and tcltest::test.  The
contract is, "I supply you with a valid test and you run the
test".  If the contract cannot be fulfilled, that is, if the
test can't be run, then an error is thrown.

I believe strongly that this contract is a more robust way
to construct a large suite of tests.  Imagine that it
weren't this way; how do I verify that my tests are valid? 
Parse the output through the errorChannel?  That is not
robust.  Wrap each call to 'test' in an 'if'?  This is not
practical and so nobody does this.  Out of 10,000 tests in
the tcl suite, only 2 calls (in tcltest.test) were checking
the return status of 'test'.

Tcltest has been greatly enhanced in 2.0, and it remains an
excellent tool for validating software correctness.  I would
be disappointed to see it compromised by allowing broken
software to pass through its tests without anyone noticing
the problem.

Regards,
Ken Cox

Logged In: YES 
user_id=246646

Darn those expiring pages!  Sorry for the repeat.  
Apologies also to Donal for mixing his ideas up with Don's.

Logged In: YES 
user_id=246646

Don, I like your patch better, because it doesn't duplicate 
the error strings, and it has the bonus of printing out the 
valid flags when you supply a bad one.  I don't know what 
the original purpose was of printing the error to 
[errorChannel], but I'm happy to avoid it.

I'm also OK with your latest suggestion: record the number 
of tests failing due to syntax errors.  I would be happy to 
count these error conditions in the Failed column.  The 
main issue for myself, and I suspect for the guy with the 
11,293 tests, is that I don't want to accidentally miss 
programming errors.  This is why I write tests in the first 
place.

I don't believe that leaving things as they are is a 
reasonable alternative.  Let's say test 137 out of 11,293 
fails due to a syntax error.  How would you notice?  
Currently, the only record you have that anything went 
wrong is some small bit of output in a potential sea of 
output.  There are things that you could do to notice them, 
such as wrapping tcltest::test in your own proc, or 
wrapping puts, or creating a stacked errorChannel, but I 
don't think any of them are as useful, or as easy, as 
throwing an exception.

My first preference is to take Don's patch as is.  This 
patch treats programming errors as exceptions, which is 
standard practice for tcl libraries.  For instance, the 
scripts {array names} and {file} throw exceptions due to 
usage errors.

My second preference is to implement Don's suggestion of 
recording the number of syntax errors.  This is less 
attractive to me because it is a more invasive change.  
Having a new element in the tcltest::numTests array would 
be OK and straightforward, though it would have the 
unfortunate side effect of making the summary output from 
tcltest::cleanupTests harder to read; it probably would no 
longer fit on one line.  Having syntax errors counted under 
the Failed column would be better, though I fear it would 
be more invasive to the existing control structure.

Logged In: YES 
user_id=246646

Don, I like your patch better, because it doesn't duplicate 
the error strings, and it has the bonus of printing out the 
valid flags when you supply a bad one.  I don't know what 
the original purpose was of printing the error to 
[errorChannel], but I'm happy to avoid it.

I'm also OK with your latest suggestion: record the number 
of tests failing due to syntax errors.  I would be happy to 
count these error conditions in the Failed column.  The 
main issue for myself, and I suspect for the guy with the 
11,293 tests, is that I don't want to accidentally miss 
programming errors.  This is why I write tests in the first 
place.

I don't believe that leaving things as they are is a 
reasonable alternative.  Let's say test 137 out of 11,293 
fails due to a syntax error.  How would you notice?  
Currently, the only record you have that anything went 
wrong is some small bit of output in a potential sea of 
output.  There are things that you could do to notice them, 
such as wrapping tcltest::test in your own proc, or 
wrapping puts, or creating a stacked errorChannel, but I 
don't think any of them are as useful, or as easy, as 
throwing an exception.

My first preference is to take Don's patch as is.  This 
patch treats programming errors as exceptions, which is 
standard practice for tcl libraries.  For instance, the 
scripts {array names} and {file} throw exceptions due to 
usage errors.

My second preference is to implement Don's suggestion of 
recording the number of syntax errors.  This is less 
attractive to me because it is a more invasive change.  
Having a new element in the tcltest::numTests array would 
be OK and straightforward, though it would have the 
unfortunate side effect of making the summary output from 
tcltest::cleanupTests harder to read; it probably would no 
longer fit on one line.  Having syntax errors counted under 
the Failed column would be better, though I fear it would 
be more invasive to the existing control structure.

Logged In: YES 
user_id=79902

It might be an idea to print a count of tests that failed
due to syntax errors in the call to tcltest at the end,
similar to the count of test failures currently printed. 
(This is in addition to the proposed printing of a message
complaining about it.)

Another (not really related) point: Does tcltest2 warn about
duplicate test names?  It would be tremendous if it did,
since I've encountered them a few times in the Tcl test
suite and they can make tracking a bug down quite a bit
harder than it ought to be.  Note that the problem is not
just within a single test file, but across test files...

Logged In: YES 
user_id=225959

Hi Don,
It's Melissa Chawla, the (woefully delinquent) maintainer of
this package!  I'm sorry for not logging in before.  I had
thought that my email address (which includes my full name)
would be included in my message, but it wasn't.

As for reading the patches, I didn't, and I'm sorry for
that.  When I started my message, the patch wasn't uploaded
yet.  I guess I don't really have much sympathy for folks
who call the test command with bad args.  You can run
procheck over your code to catch such problems (I've used a
tcltest.pcx extention to procheck to achieve this in the
past).  Although if you're generating your test calls,
procheck won't help.

I'm all for improving the docs so they fully reflect what
the code does.

I'm not totally opposed to the last suggestion of having an
tcltest package option to disallow the test command from
ever throwing an exception, though I wonder how many other
people will really use it.  In the mean time, you can
achieve this by doing something like the following
(untested) code:

package require tcltest 2.0
rename ::tcltest::test ::tcltest::test_old
proc ::tcltest::test {args} {
      if {[catch {eval ::tcltest::test_old $args} result]} {
        puts "bad call to test command with args: $args:
$::errorInfo"
    }
    return $result
}

Hopefully this second round of comments is more helpful to
folks.
-Melissa

Logged In: YES 
user_id=80530

You know, sometimes I just lack imagination.

Ken, here's an example where the existing non-error-throwing
behavior of [test] could be important.  Imagine you are
using tcltest as the engine to drive a very large,
very expensive, unattended set of tests.  Maybe I'm
testing some software that runs only on a supercomputer
where I only get access by having my process wait in a
batch queue for a paid slot of processing time in the
middle of the night.  Say I make that same typo in
test 137 out of 11,293.  I'll be unhappy the next day to
learn that test 137 did not succeed because of my stupid
typo, but I'll be devastated if tests 138-11,293 did not
even run because of it.

So for someone using tcltest in that way, they will
prefer the current tcltest 2 way of doing things, over
the old tcltest 1 behavior.  They may even have moved from
version 1 to version 2 to get that feature.  And they'll
be very unhappy to see it change.

So it seems to me the best way to cater to everyone is
to make this aspect of [test] configurable.  Add some
new command in tcltest by which you can configure whether
or not you want [test] to throw error when it is called
incorrectly, with the default that it does not.

Thoughts?

Logged In: YES 
user_id=80530

Who is this that just replied?  Please take the
time to log in when commenting.

Did you look at either patch to see what they do?

The documentation says nothing that indicates to me
an inability of [test] to throw an error when it's
not called properly according to its documentation.
In fact, the script [test name] with only one argument
*does* raise an error, as does the script [test] with no
arguments.  These patches do nothing contrary to
the documentation.

Everyone who's contributed here so far agrees that
extending [test] to accept more options in tcltest
version 2.x was a good thing.  (A *very* good thing!)
Don't raise a dispute over points that are not in
dispute.

Regarding compatibility, can you cite any examples
of people making use of the fact that [test] will 
currently not raise an error when called incorrectly?
Why do they want to call [test] incorrectly in the first
place?  Why would that ever be intentional?  And since it's
not intentional, why wouldn't they want to get notified
about their unintentional error so it can be corrected?

As an example, if I have:

test switch-0.0 {broken example} -match glub -body {
    switch
} -result {wrong # args:*}

why don't I want an error to alert me to my typo
(glub for glob)?

Logged In: NO 

By the logic that you are using, no command should ever be
extended to take more arguments because previously erroneous
calls now succeed!

As far as I can tell, tcltest::test works as documented, and
it's an improvement to take more args than before.

The correct definition of a backwards-incompatible change is
one in which previously accepted behavior is nolonger
valid.  An accepted "Tcl-ism" is that one should never rely
on upwards compatibility of error conditions.

Also, If we change the behavior now, folks whose code relied
upon the command not throwing an error (a reasonable
expectation, given the docs) will have broken code.

Given all this, I don't think this patch should be applied.

File Added - 17453: tcltest.patch

Logged In: YES 
user_id=80530

I agree with your assessment.  What do you think
of this alternative patch?

File Added - 17433: patch

Logged In: YES 
user_id=246646

Doh!  I did forget to check the box.  Thank you.

Note that tcl bug 497446 is different to my mind.  Bug 
497446 says that tcltest::test does not work as 
documented.  This bug says that how it is documented is 
broken and is a regression.

Thanks for the note about tcltest 1, but I really do like 
and appreciate the extensions and improvements in tcltest 2.

Logged In: YES 
user_id=80530

BTW, there's no patch attached here.  Did you
forget to check the box?

Logged In: YES 
user_id=80530

See also Tcl Bug 497446.

Note that if you require tcltest version 1
behavior, you can always [package require tcltest 1].