Re: Trying to add more tests to gistbuild.c

From: Aleksander Alekseev <aleksander(at)timescale(dot)com>
To: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Cc: Matheus Alcantara <mths(dot)dev(at)pm(dot)me>
Subject: Re: Trying to add more tests to gistbuild.c
Date: 2022-07-27 13:07:46
Message-ID: CAJ7c6TNT8ZE0_eGRSad4zJ8O-A5qeVmpONMwtF543RKGeBGyjg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Matheus,

Many thanks for hacking on increasing the code coverage! I noticed
that this patch was stuck in "Needs Review" state for some time and
decided to take a look.

> With these new tests the coverage went from 45.3% to 85.5%, but I have some doubts:
> - Does this test make sense?
> - Would there be a way to validate that the buffering was done correctly?
> - Is this test necessary?

I can confirm that the coverage improved as stated.

Personally I believe this change makes perfect sense. Although this is
arguably not an ideal test for gistInitBuffering(), writing proper
tests for `static` procedures is generally not an easy task. Executing
the code at least once in order to make sure that it doesn't crash,
doesn't throw errors and doesn't violate any Asserts() is better than
doing nothing.

Here is a slightly modified patch with added commit message. Please
note that patches created with `git format-patch` are generally
preferable than patches created with `git diff`.

I'm going to change the status of this patch to "Ready for Committer"
in a short time unless anyone has a second opinion.

--
Best regards,
Aleksander Alekseev

Attachment Content-Type Size
v3-0001-Improve-test-coverage-of-gistbuild.c.patch application/octet-stream 2.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2022-07-27 13:47:38 Re: fairywren hung in pg_basebackup tests
Previous Message Amit Langote 2022-07-27 12:50:23 Re: Skip partition tuple routing with constant partition key