On Wed, Jun 23, 2021 at 08:26:32AM +0200, Fabien COELHO wrote:
>> Could we make that shorter at 1s? That will shorten the duration of
>> the test run. It is easy to miss that this test is for
>> --aggregate-interval (aka the logAgg() path) without a comment.
>
> It is for -T, -P and --aggregate-interval. The units of all these is
> seconds, the minimum is 1, I put 2 so that It is pretty sure to get some
> output. We could try 1, but I'm less confident that an output is ensured in
> all cases on a very slow host which may decide to shorten the run before
> having shown a progress for instance.
Could it be possible to document the intention of the test and its
coverage then? With the current patch, one has to guess what's the
intention behind this case.
>> +# $nthreads threads, 2 seconds, but due to timing imprecision we might get
>> +# only 1 or as many as 3 progress reports per thread.
>> +check_pgbench_logs($bdir, '001_pgbench_log_1', $nthreads, 1, 3,
>> + qr{^\d{10,} \d{1,2} \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+$});
>> +
>> Now this one is good and actually stable thanks to the fact that we'd
>> get at least the final logs, and the main complain we got to discuss
>> about on this thread was the format of the logs.
>
> Yep, this test would have probably detected the epoch issue reported by
> Greg.
(Sorry I missed the use of throttle_delay that would generate 10
fields in the log file)
Hm.. Could it be possible to tighten a bit the regex used here then?
I was playing with it and it is not really picky in terms of patterns
allowed or rejected. The follow-up checks for check_pgbench_logs()
could be a bit more restrictive as well, but my regex-fu is bad.
>> I would say to give up on the first test, and keep the second.
>
> You mean remove the time check and keep the log check. I'd like to keep a
> time check, or at least have it in comment so that I can enable it simply.
I'd rather avoid tests that tend to fail on slow machines. There is a
flotilla in the buildfarm.
--
Michael
