On white-box tests

On white-box tests

[Ludovic Courtès]

Hello!

Just a note that I've been meaning to send for some time.

"Michael Gran" <spk121@yahoo.com> writes:

> http://git.savannah.gnu.org/cgit/guile.git/commit/?id=9b0c25f6d18d5be318ea3a47fd87cf7e63b689e1

[...]

> --- a/test-suite/tests/strings.test
> +++ b/test-suite/tests/strings.test

[...]

> +;;
> +;; string internals
> +;;
> +
> +;; Some abbreviations
> +;; BMP - Basic Multilingual Plane (codepoints below U+FFFF)
> +;; SMP - Suplementary Multilingual Plane (codebpoints from U+10000 to U+1FFFF)
> +
> +(with-test-prefix "string internals"
> +
> +  (pass-if "new string starts at 1st char in stringbuf"
> +    (let ((s "abc"))
> +      (= 0 (assq-ref (%string-dump s) 'start))))
> +
> +  (pass-if "length of new string same as stringbuf"
> +    (let ((s "def"))
> +      (= (string-length s) (assq-ref (%string-dump s) 'stringbuf-length))))
> +
> +  (pass-if "contents of new string same as stringbuf"
> +    (let ((s "ghi"))
> +      (string=? s (assq-ref (%string-dump s) 'stringbuf-chars))))
> +
> +  (pass-if "writable strings are not read-only"
> +    (let ((s "zyx"))
> +      (not (assq-ref (%string-dump s) 'read-only))))
> +
> +  (pass-if "read-only strings are read-only"
> +    (let ((s (substring/read-only "zyx" 0)))
> +      (assq-ref (%string-dump s) 'read-only)))
> +
> +  (pass-if "null strings are inlined"
> +    (let ((s ""))
> +      (assq-ref (%string-dump s) 'stringbuf-inline)))

First of all, thanks for taking the time to write unit tests!

I'm not fully convinced by some of these "string internals" tests,
though.  These are "white box tests".  I believe these tests are mostly
useful when written by someone different than the one who implemented
the code under test, both of whom following a given specification.  When
someone writes both the code and the white box tests, I'm afraid they
end up writing the same code twice, just differently.  For example:

  SCM
  scm_i_substring_read_only (SCM str, size_t start, size_t end)
  {
    [...]

    return scm_double_cell (RO_STRING_TAG,  /* <--- read-only */
                            SCM_UNPACK(buf),
                            (scm_t_bits)str_start + start,
                            (scm_t_bits) end - start);
  }

and:

  (pass-if "read-only strings are read-only"
    (let ((s (substring/read-only "zyx" 0)))
      (assq-ref (%string-dump s) 'read-only)))

Thus, I think such tests don't provide much information.

Conversely, for this example, a black-box test of the public API would
have helped catch regressions and non-conformance issues:

  (pass-if-exception exception:read-only-string
    (string-set! (substring/read-only "zyx" 0) 1 #\x))

Another issue is that these tests expose a lot of the implementation
details, e.g.:

  (pass-if "short Latin-1 encoded strings are inlined"
    (let ((s "m"))
      (assq-ref (%string-dump s) 'stringbuf-inline)))

The inline/outline distinction is an implementation detail.  If we
change it (that's something we could get rid of in the BDW-GC branch,
for instance) then the tests will have to be rewritten or removed.

Conversely, black box tests can give confidence that a change in
implementation details did have any user-visible impact.

There *are* situations where the gap between the public API and the
internals is too important, and white box tests can come in handy here
(that's why `%string-dump' et al. are nice tools).  However, often, I
think it's better to have good coverage of the public API than a wealth
of "obvious" white-box tests.

What do you think?

Thanks,
Ludo'.

Re: On white-box tests

[Mike Gran]

On Wed, 2009-08-19 at 10:38 +0200, Ludovic Courtès wrote:
> Hello!
> 
> Just a note that I've been meaning to send for some time.
> 
> "Michael Gran" <spk121@yahoo.com> writes:
> 
> > http://git.savannah.gnu.org/cgit/guile.git/commit/?id=9b0c25f6d18d5be318ea3a47fd87cf7e63b689e1
> 
> [...]
> 

> I'm not fully convinced by some of these "string internals" tests,
> though.  These are "white box tests".  I believe these tests are mostly
> useful when written by someone different than the one who implemented
> the code under test, both of whom following a given specification.  When
> someone writes both the code and the white box tests, I'm afraid they
> end up writing the same code twice, just differently.  For example:

I know what your saying.  I was just a kid with a new toy.

In my line of work, where 100% code coverage of test suites is a common
requirement, we end up writing many such overly obvious tests.  They are
often the only way to hit some internal branches.  They are often
pointless.  

> What do you think?

Keep it or dump it.  It's all good.

> 
> Thanks,
> Ludo'.

Thanks,

Mike

Re: On white-box tests

[Ludovic Courtès]

Mike Gran <spk121@yahoo.com> writes:

> On Wed, 2009-08-19 at 10:38 +0200, Ludovic Courtès wrote:

[...]

>> What do you think?
>
> Keep it or dump it.  It's all good.

Just to be clear: I think we can keep them, it doesn't hurt.

I just wanted to hear what you and others thought about the issue,
because I think unit tests are a crucial part of software development.

Thanks,
Ludo'.

Re: On white-box tests

[Mike Gran]

On Wed, 2009-08-19 at 15:53 +0200, Ludovic Courtès wrote:
> Mike Gran <spk121@yahoo.com> writes:
> 
> > On Wed, 2009-08-19 at 10:38 +0200, Ludovic Courtès wrote:

> I just wanted to hear what you and others thought about the issue,
> because I think unit tests are a crucial part of software development.

OK.  To say something slightly more cogent.  I think when a current
phase of development centers around modifying low-level code, it is
useful to have a set of low-level tests.  If those tests fail, it
reminds the developer that s/he has modified something upon which other
routines rely.

I wrote the string-internals tests to indicate to me when I'd done
something that had unexpected side-effects.  They are intentionally
white-box; they are intentionally reflexive.

There is a danger that those tests, should they remain, could be seen as
indicating software requirements, which they do not.  The software
requirement specification for Scheme (RnRS) is high level and leaves
much of the implementation detail unspecified.

I think it is a good idea to leave them in, probably with comments that
express that they test the implementation, not the specification.   I
also think that it is a good idea to segregate them from tests that
exercise the actual software requirements.

But, I can see an equally valid argument for stripping them out once
strings are no longer in flux, for example at release 2.0, assuming it
is bug free ;-) or perhaps 2.1.

Thanks,

Mike




> 
> Thanks,
> Ludo'.
> 
> 
> 

Re: On white-box tests

[Ludovic Courtès]

Mike Gran <spk121@yahoo.com> writes:

> I wrote the string-internals tests to indicate to me when I'd done
> something that had unexpected side-effects.  They are intentionally
> white-box; they are intentionally reflexive.

OK, I understand now.  Then if it's useful to you, well, it's indeed
useful.  :-)

> But, I can see an equally valid argument for stripping them out once
> strings are no longer in flux,

Makes sense.

> for example at release 2.0, assuming it is bug free ;-) or perhaps
> 2.1.

But I'm sure it'll all be ready by 2.0 or even 1.9.3!  :-)

Thanks,
Ludo'.