Re: Automated report: NetBSD-current/i386 test failure

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Wed, 25 Oct 2017 17:52:52 +0000 (UTC)
    From:        NetBSD Test Fixture <bracket%NetBSD.org@localhost>
    Message-ID:  <150895397201.5616.843588977796354456%babylon5.NetBSD.org@localhost>

  | The newly failing test cases are:
  | 
  |     lib/libc/time/t_mktime:mktime_negyear

I have been looking at this one (first) (it was first in the list).

To me it looks like it is a bogus test, and I have no idea how it
ever worked.

The test (it is short) is (boilerplate omitted)...

        struct tm tms;
        time_t t;

        (void)memset(&tms, 0, sizeof(tms)); 
        tms.tm_year = ~0;

        errno = 0;
        t = mktime(&tms);
        ATF_REQUIRE_ERRNO(0, t != (time_t)-1);

That is, stick in a negative year (no idea why ~0 rather than -1, but
that is not the problem).   Run mktime().

Then we test that the result is not -1 (which ends up true, it is not -1),
it is some other big negative number, which when I pass to "date -d @NNNN"
looks like it is correct for such a badly uninitialised struct tm.

If the boolean 2nd arg to ATF_REQUIRE_ERRNO is true, the macro (or the
function it calls) checks that errno == the first arg.

But that's nonsense - if mktime() returns -1, it sets errno to indicate
the error, if it does not return -1, errno is undefined (nothing promises
that it will not be altered, and in fact it is - the first alteration I
see if when mktime() (via a function it calls) calls malloc() which checks
for /etc/malloc.conf (or some file like that) which does not exist, and
gets errno set because of that.  That's not an error, just malloc() doing
its thing.   But errno remains showing that error.   mktime() (other
functions it calls) can also set errno for other internal purposes.)

I'd expect ATF_REQUIRE_ERRNO() to be used when testing for expected known
failure, like if you were to do

	res = unlink("/tmp/no/such/file");

you'd expect res == -1, and errno == ENOENT, so you'd write

	ATF_REQUIRE_ERRNO(ENOENT, res == -1);

Then if the unlink() didn't fail (res was not -1) the test fails,
otherwise (res == -1) we check to see that the reason it failed was
what we expect (ENOENT) and not some other reason - if it is, then
all is good, and the test passes, if errno is some other value we fail.

I am not sure what code changed in tzcode2017c which is now causing
this difference in behaviour, I diff'd old and new, and none of the
changed lines mention errno anywhere, so it isn't as if the old
version was preserving errno (unnecessarily) and that's no longer
happening.

Do those of you who know ATF better than I (which probably means the
vast majority of you) agree with this analysis?

If so, I'll take my butcher's tools to the tests, and try to turn them
into something more reasonable, if someone else doesn't get at them first.

kre

ps: I have not investigated the failures other than in lib/libc/time/t_mktime
yet, but both sub-tests that are failing there look to have the same issue.