Thread: Re: Rewriting the test of pg_upgrade as a TAP test

Craig, Michael,

* Craig Ringer (craig@2ndquadrant.com) wrote:
> On 3 April 2017 at 21:07, Michael Paquier <michael.paquier@gmail.com> wrote:
> > I am parking that in the next commit fest.
>
> Great.
>
> Count me in as reviewer, and feel free to poke me if I get caught up
> in other things.

I'm interested in this also.

> I'd like to see us adopting TAP for cross-version stuff in pg_dump etc
> too, down the track.

I'm very curious what you're thinking here?  IIRC, Andrew had some ideas
for how to do true cross-version testing with TAP in the buildfarm, but
I don't think we actually have that yet..?

Thanks!

Stephen

On Tue, Apr 4, 2017 at 12:12 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I'm very curious what you're thinking here?  IIRC, Andrew had some ideas
> for how to do true cross-version testing with TAP in the buildfarm, but
> I don't think we actually have that yet..?

I heard about nothing in this area. Cross-branch tests may be an
interesting challenge as tests written in branch X may not be in Y.
The patch presented here does lower the coverage we have now.
-- 
Michael

On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Not good if it lowers the coverage, but hopefully that's fixable. Have you
> analyzed where we're reducing coverage..?

The current set of tests is just running pg_upgrade using the same
version for the source and target instances. Based on that I am not
lowering what is happening in this set of tests. Just doing some
cleanup.

> As for what I'm remembering, there's this:
> https://www.postgresql.org/message-id/5669acd9-efdc-2a0f-afea-10ba6003a050@dunslane.net
>
> Of course, it's possible I misunderstood..

This invokes directly pg_upgrade, so that's actually a third,
different way to test pg_upgrade on top of the two existing methods
that are used in vcregress.pl and pg_upgrade's test.sh

> That seems focused on upgrading and I'd really like to see a general way to
> do this with the TAP structure, specifically so we can test pg_dump and psql
> against older versions.  Having the ability to then be run under the
> coverage testing would be fantastic and would help a great deal with the
> coverage report.

I don't disagree with that. What we need first is some logic to store
in a temporary directory the installation of all the previous major
versions that we have. For example use a subfolder in tmp_install
tagged with the major version number, and then when the TAP test
starts we scan for all the versions present in tmp_install and test
the upgrade with a full grid. One issue though is that we add
$(bindir) in PATH and that there is currently no logic to change PATH
automatically depending on the major/minor versions you are working
on.

So in short I don't think that this lack of infrastructure should be a
barrier for what is basically a cleanup but... I just work here.
-- 
Michael

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > Not good if it lowers the coverage, but hopefully that's fixable. Have you
> > analyzed where we're reducing coverage..?
>
> The current set of tests is just running pg_upgrade using the same
> version for the source and target instances. Based on that I am not
> lowering what is happening in this set of tests. Just doing some
> cleanup.

Ok, I'm confused.

I wrote the above in response to your statement:

> The patch presented here does lower the coverage we have now.

I assume (perhaps mistakenly) that this statement was based on an
analysis of before-and-after 'make coverage' runs.  Here you are saying
that you're *not* lowering the coverage.

I understand how the current pg_upgrade tests work.  I don't see
off-hand why the TAP tests would reduce the code coverage of pg_upgrade,
but if they do, we should be able to figure out why and correct it.

> > As for what I'm remembering, there's this:
> > https://www.postgresql.org/message-id/5669acd9-efdc-2a0f-afea-10ba6003a050@dunslane.net
> >
> > Of course, it's possible I misunderstood..
>
> This invokes directly pg_upgrade, so that's actually a third,
> different way to test pg_upgrade on top of the two existing methods
> that are used in vcregress.pl and pg_upgrade's test.sh

Ok, though I'm not sure that I see that as necessairly a bad thing.
There are only specific tools that we actually worry about being able to
work with older versions of PG, after all.

> > That seems focused on upgrading and I'd really like to see a general way to
> > do this with the TAP structure, specifically so we can test pg_dump and psql
> > against older versions.  Having the ability to then be run under the
> > coverage testing would be fantastic and would help a great deal with the
> > coverage report.
>
> I don't disagree with that. What we need first is some logic to store
> in a temporary directory the installation of all the previous major
> versions that we have. For example use a subfolder in tmp_install
> tagged with the major version number, and then when the TAP test
> starts we scan for all the versions present in tmp_install and test
> the upgrade with a full grid. One issue though is that we add
> $(bindir) in PATH and that there is currently no logic to change PATH
> automatically depending on the major/minor versions you are working
> on.

Right, I figured that what Andrew did in the above post was something
along these lines, but I've not looked at it in any depth.

> So in short I don't think that this lack of infrastructure should be a
> barrier for what is basically a cleanup but... I just work here.

I didn't mean to imply that this patch needs to address the
cross-version testing challenge, was merely mentioning that there's been
some work in this area already by Andrew and that if you're interested
in working on that problem that you should probably coordinate with him.

What I do think is a barrier to this patch moving forward is if it
reduces our current code coverage testing (with the same-version
pg_upgrade that's run in the regular regression tests).  If it doesn't,
then great, but if it does, then the patch should be updated to fix
that.

Thanks!

Stephen

On Tue, Apr 4, 2017 at 9:30 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> The patch presented here does lower the coverage we have now.
>
> I assume (perhaps mistakenly) that this statement was based on an
> analysis of before-and-after 'make coverage' runs.  Here you are saying
> that you're *not* lowering the coverage.

My apologies here, when I used the work "coverage" in my previous
emails it visibly implied that I meant that I had used the coverage
stuff. But I did not because the TAP test proposed does exactly what
test.sh is doing:
1) Initialize the old cluster and start it.
2) create a bunch of databases with full range of ascii characters.
3) Run regression tests.
4) Take dump on old cluster.
4) Stop the old cluster.
5) Initialize the new cluster.
6) Run pg_upgrade.
7) Start new cluster.
8) Take dump from it.
9) Run deletion script (Oops forgot this part!)

> I understand how the current pg_upgrade tests work.  I don't see
> off-hand why the TAP tests would reduce the code coverage of pg_upgrade,
> but if they do, we should be able to figure out why and correct it.

Good news is that this patch at least does not lower the bar.

>> So in short I don't think that this lack of infrastructure should be a
>> barrier for what is basically a cleanup but... I just work here.
>
> I didn't mean to imply that this patch needs to address the
> cross-version testing challenge, was merely mentioning that there's been
> some work in this area already by Andrew and that if you're interested
> in working on that problem that you should probably coordinate with him.

Sure.

> What I do think is a barrier to this patch moving forward is if it
> reduces our current code coverage testing (with the same-version
> pg_upgrade that's run in the regular regression tests).  If it doesn't,
> then great, but if it does, then the patch should be updated to fix
> that.

I did not do a coverage test first, but surely this patch needs
numbers, so here you go.

Without the patch, here is the coverage of src/bin/pg_upgrade:
  lines......: 57.7% (1311 of 2273 lines)
  functions..: 85.3% (87 of 102 functions)

And with the patch:
  lines......: 58.8% (1349 of 2294 lines)
  functions..: 85.6% (89 of 104 functions)
The coverage gets a bit higher as a couple of basic code paths like
pg_upgrade --help get looked at.
-- 
Michael

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Tue, Apr 4, 2017 at 9:30 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Michael Paquier (michael.paquier@gmail.com) wrote:
> >> On Tue, Apr 4, 2017 at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
> >> The patch presented here does lower the coverage we have now.
> >
> > I assume (perhaps mistakenly) that this statement was based on an
> > analysis of before-and-after 'make coverage' runs.  Here you are saying
> > that you're *not* lowering the coverage.
>
> My apologies here, when I used the work "coverage" in my previous
> emails it visibly implied that I meant that I had used the coverage
> stuff. But I did not because the TAP test proposed does exactly what
> test.sh is doing:

Ah, ok, no worries.  Glad to hear that there isn't any difference in
coverage or in what's being done.

> 1) Initialize the old cluster and start it.
> 2) create a bunch of databases with full range of ascii characters.
> 3) Run regression tests.
> 4) Take dump on old cluster.
> 4) Stop the old cluster.
> 5) Initialize the new cluster.
> 6) Run pg_upgrade.
> 7) Start new cluster.
> 8) Take dump from it.
> 9) Run deletion script (Oops forgot this part!)

Presumably the check to match the old dump against the new one is also
performed?

> > I understand how the current pg_upgrade tests work.  I don't see
> > off-hand why the TAP tests would reduce the code coverage of pg_upgrade,
> > but if they do, we should be able to figure out why and correct it.
>
> Good news is that this patch at least does not lower the bar.

Great, then I don't see any reason we can't move forward with it.

> > What I do think is a barrier to this patch moving forward is if it
> > reduces our current code coverage testing (with the same-version
> > pg_upgrade that's run in the regular regression tests).  If it doesn't,
> > then great, but if it does, then the patch should be updated to fix
> > that.
>
> I did not do a coverage test first, but surely this patch needs
> numbers, so here you go.
>
> Without the patch, here is the coverage of src/bin/pg_upgrade:
>   lines......: 57.7% (1311 of 2273 lines)
>   functions..: 85.3% (87 of 102 functions)
>
> And with the patch:
>   lines......: 58.8% (1349 of 2294 lines)
>   functions..: 85.6% (89 of 104 functions)
> The coverage gets a bit higher as a couple of basic code paths like
> pg_upgrade --help get looked at.

Fantastic, that's even better.

Thanks!

Stephen

On Wed, Apr 5, 2017 at 10:24 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> 1) Initialize the old cluster and start it.
>> 2) create a bunch of databases with full range of ascii characters.
>> 3) Run regression tests.
>> 4) Take dump on old cluster.
>> 4) Stop the old cluster.
>> 5) Initialize the new cluster.
>> 6) Run pg_upgrade.
>> 7) Start new cluster.
>> 8) Take dump from it.
>> 9) Run deletion script (Oops forgot this part!)
>
> Presumably the check to match the old dump against the new one is also
> performed?

Yes. That's run with command_ok() at the end.
-- 
Michael