Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2740) Bug#38133

[Guilhem Bichot]

Hello,

Ingo Strüwing a écrit, Le 12/17/2008 12:00 PM:
> Hi Guilhem,
> 
> thanks for the review! Please see below.
> 
> Guilhem Bichot, 15.12.2008 14:23:
> ...
>> Ingo Struewing a écrit, Le 12/10/2008 07:43 PM:
> ...
>>>  2740 Ingo Struewing    2008-12-10
>>>       Bug#38133 - Myisamlog test fails on Windows
> ...
>>> === added file 'mysql-test/r/myisamlog_coverage.result'
>>> --- a/mysql-test/r/myisamlog_coverage.result    1970-01-01 00:00:00 +0000
>>> +++ b/mysql-test/r/myisamlog_coverage.result    2008-12-10 18:43:34 +0000
>> It is impressive to see so much testing of error conditions.
>> I remember some rule somewhere saying that all devs have to please dgcov
>> (a.k.a. have good coverage of their push), my hat off to you for being
>> one of the few (or maybe the only one) to honour this rule. It does give
>> a feeling of confidence that you tested your work!!
> 
> Thanks. When we introduced this rule, I thought it was stupid to spend
> so much time in testing every branch. After all, one catches a very
> small class of problems only. But when I practiced it the first time, I
> found a couple of problems, which I didn't expect to happen. This
> encouraged me, to try it again. And yes, the same thing happened again.
> I did even find crash paths. Adding tests that try to trigger a code
> path, often reveals unexpected behavior of the code. Even when this
> turns out to be correct, I do at least learn something.
> 
> So in summary, I learned that coverage testing is not a waste of time,
> but pays off.
> 
> But I don't believe to be the only one doing it. If I remember
> correctly, Monty was a strong advocate for this rule. So I believe, he
> is doing it too.

Maybe he does, I don't know. You're the only one I see who covers error 
paths in tests.

>>> === added file 'mysql-test/t/myisamlog_coverage.test'
>>> --- a/mysql-test/t/myisamlog_coverage.test    1970-01-01 00:00:00 +0000
>>> +++ b/mysql-test/t/myisamlog_coverage.test    2008-12-10 18:43:34 +0000
>>> @@ -0,0 +1,128 @@
> ...
>>> +# NOTE: This test cannot run together with myisamlog.test.
> ...
>> We had the same problem with tests reading the Maria transaction log and
> ...
>> (for an example: see suite/maria/t/maria-no-logging.test in the latest
> 
> Ok. Thanks for the tip. I made it working.
> 
> However, I would have preferred some statement similar to FLUSH LOGS. If
> myisam.log grows too big, there is no way around a server restart with
> the current implementation.

Indeed.

> After testing on Windows, it turned out that there is a flaw in the
> server logging code. At least one of the tests creates a temporary
> MEMORY table, which is later converted into a file based table.

And as you explained, it's "SHOW STATUS" which causes that.

> On
> Linux, Maria is used, but on Windows MyISAM is used.

The difference must be because that Windows build didn't include Maria.

> MyISAM logs open of
> the temporary table. When myisamlog tries to execute the log file, two
> problems happen:
> 
> 1. The temp file name is an absolute path and is appended to the data
> directory. This was pretty easy to fix.
> 
> 2. myisamlog cannot open the temporary table, because the table files
> don't exist any more at this time.
> 
> So I wonder if it is correct to log temporary table actions at all? I
> would happily exclude temporary tables, but this is probably beyond the
> approval for this patch.

Let's see if Monty is ok with this on IRC, and if yes, do this.

> An (ugly) alternative would be to add include/not_windows to the test
> case. Or even better (but still ugly) include/have_maria.

have_maria is a bit different from "is Maria used for tmp tables". It is 
possible to have Maria compiled in but not used for tmp tables.
And not_windows will fail if there is a Unix build without Maria.