Horton

Thread

Commits

  1. pg_waldump: Add test case for notice message

  2. pg_waldump: Add more tests

  1. pg_waldump: add test for coverage

    Dong Wook Lee <sh95119@gmail.com> — 2022-08-23T01:50:08Z

    Hi Hackers,
I wrote a test for coverage.
Unfortunately, it seems to take quite a while to run the test.
I want to improve these execution times, but I don't know exactly what to do.
Therefore, I want to hear feedback from many people.
---
Regards,
Dong Wook Lee

  2. Re: pg_waldump: add test for coverage

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-06T05:57:57Z

    On 23.08.22 03:50, Dong Wook Lee wrote:
> Hi Hackers,
> I wrote a test for coverage.
> Unfortunately, it seems to take quite a while to run the test.
> I want to improve these execution times, but I don't know exactly what to do.
> Therefore, I want to hear feedback from many people.

I don't find these tests to be particularly slow.  How long do they take 
for you to run?

A couple of tips:

- You should give each test a name.  That's why each test function has a 
(usually) last argument that takes a string.

- You could use command_like() to run a command and check that it exits 
successfully and check its standard out.  For example, instead of

# test pg_waldump with -F (main)
IPC::Run::run [ 'pg_waldump', "$wal_dump_path", '-F', 'main' ], '>', 
\$stdout, '2>', \$stderr;
isnt($stdout, '', "");

it is better to write

command_like([ 'pg_waldump', "$wal_dump_path", '-F', 'main' ],
              qr/TODO/, 'test -F (main)');

- It would be useful to test the actual output (that is, fill in the 
TODO above).  I don't know what the best way to do that is -- that is 
part of designing these tests.

Also,

- Your patch introduces a spurious blank line at the end of the test file.

- For portability, options must be before non-option arguments.  So 
instead of

[ 'pg_waldump', "$wal_dump_path", '-F', 'main' ]

it should be

[ 'pg_waldump', '-F', 'main', "$wal_dump_path" ]


I think having some more test coverage for pg_waldump would be good, so 
I encourage you to continue working on this.

  3. Re: pg_waldump: add test for coverage

    Andres Freund <andres@anarazel.de> — 2022-09-22T15:16:10Z

    Hi,

On 2022-08-23 10:50:08 +0900, Dong Wook Lee wrote:
> I wrote a test for coverage.

Unfortunately the test doesn't seem to pass on windows, and hasn't ever done so:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3834

Due to the merge of the meson patchset, you should also add 001_basic.pl to
the list of tests in meson.build

Greetings,

Andres Freund

  4. Re: pg_waldump: add test for coverage

    Peter Eisentraut <peter@eisentraut.org> — 2023-06-14T07:16:50Z

    On 06.09.22 07:57, Peter Eisentraut wrote:
>> I wrote a test for coverage.
>> Unfortunately, it seems to take quite a while to run the test.
>> I want to improve these execution times, but I don't know exactly what 
>> to do.
>> Therefore, I want to hear feedback from many people.

> I think having some more test coverage for pg_waldump would be good, so 
> I encourage you to continue working on this.

I made an updated patch that incorporates many of your ideas and code, 
just made it a bit more compact, and added more tests for various 
command-line options.  This moves the test coverage of pg_waldump from 
"bloodbath" to "mixed fruit salad", which I think is pretty good 
progress.  And now there is room for additional patches if someone wants 
to figure out, e.g., how to get more complete coverage in gindesc.c or 
whatever.

  5. Re: pg_waldump: add test for coverage

    Peter Eisentraut <peter@eisentraut.org> — 2023-06-28T05:48:46Z

    On 14.06.23 09:16, Peter Eisentraut wrote:
> On 06.09.22 07:57, Peter Eisentraut wrote:
>>> I wrote a test for coverage.
>>> Unfortunately, it seems to take quite a while to run the test.
>>> I want to improve these execution times, but I don't know exactly 
>>> what to do.
>>> Therefore, I want to hear feedback from many people.
> 
>> I think having some more test coverage for pg_waldump would be good, 
>> so I encourage you to continue working on this.
> 
> I made an updated patch that incorporates many of your ideas and code, 
> just made it a bit more compact, and added more tests for various 
> command-line options.  This moves the test coverage of pg_waldump from 
> "bloodbath" to "mixed fruit salad", which I think is pretty good 
> progress.  And now there is room for additional patches if someone wants 
> to figure out, e.g., how to get more complete coverage in gindesc.c or 
> whatever.

Here is an updated patch set.  I added a test case for the "first record 
is after" message.  Also, I think this message should really go to 
stderr, since it's more of a notice or warning, so I changed it to use 
pg_log_info.

  6. Re: pg_waldump: add test for coverage

    Tristen Raab <tristen.raab@highgo.ca> — 2023-06-29T19:16:52Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Hello,

I've reviewed your latest v3 patches on Ubuntu 23.04. Both patches apply correctly and all the tests run and pass as they should. Execution time was normal for me, I didn't notice any significant latency when compared to other tests. The only other feedback I can provide would be to add test coverage to some of the other options that aren't currently covered (ie. --bkp-details, --end, --follow, --path, etc.) for completeness. Other than that, this looks like a great patch.

Kind regards,

Tristen

  7. Re: pg_waldump: add test for coverage

    Peter Eisentraut <peter@eisentraut.org> — 2023-07-05T09:01:33Z

    On 29.06.23 21:16, Tristen Raab wrote:
> I've reviewed your latest v3 patches on Ubuntu 23.04. Both patches apply correctly and all the tests run and pass as they should. Execution time was normal for me, I didn't notice any significant latency when compared to other tests. The only other feedback I can provide would be to add test coverage to some of the other options that aren't currently covered (ie. --bkp-details, --end, --follow, --path, etc.) for completeness. Other than that, this looks like a great patch.

Committed.

I added a test for the --quiet option.  --end and --path are covered.

The only options not covered now are

   -b, --bkp-details      output detailed information about backup blocks
   -f, --follow           keep retrying after reaching end of WAL
   -t, --timeline=TLI     timeline from which to read WAL records
   -x, --xid=XID          only show records with transaction ID XID

--follow is a bit tricky to test because you need to leave pg_waldump 
running in the background for a while, or something like that. 
--timeline and --xid can be tested but would need some work on the 
underlying test data (such as creating more than one timeline).  I don't 
know much about --bkp-details, so I don't have a good idea how to test 
it.  So I'll leave those as projects for the future.