Re: Add more regression tests for CREATE OPERATOR

Hi,

Please find attached a patch to take code-coverage of CREATE OPERATOR
(src/backend/commands/operatorcmds.c) from 56% to 91%.

Any and all feedback is welcome.
--
Robins Tharakan

On 23 May 2013 00:34, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:

> Hi,
>
> Please find attached a patch to take code-coverage of CREATE OPERATOR
> (src/backend/commands/operatorcmds.c) from 56% to 91%.
>
> Any and all feedback is welcome.
> --
> Robins Tharakan
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
Hi,
there is one commented out test. I think it should be run, or deleted.
There is no use of commented sql code which is not run.

What do you think?

regards,
Szymon

Thanks a ton Szymon (for a reminder on this one).

As a coincidental turn of events, I have had to travel half way across the
world and am without my personal laptop (without a linux distro etc.) and
just recovering from a jet-lag now.

I'll try to install a VM on a make-shift laptop and get something going to
respond as soon as is possible.

Thanks
--
Robins Tharakan

--
Robins Tharakan

On 17 June 2013 05:19, Szymon Guz <mabewlun(at)gmail(dot)com> wrote:

> On 23 May 2013 00:34, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:
>
>> Hi,
>>
>> Please find attached a patch to take code-coverage of CREATE OPERATOR
>> (src/backend/commands/operatorcmds.c) from 56% to 91%.
>>
>> Any and all feedback is welcome.
>> --
>> Robins Tharakan
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>>
> Hi,
> there is one commented out test. I think it should be run, or deleted.
> There is no use of commented sql code which is not run.
>
> What do you think?
>
> regards,
> Szymon
>

Hi Szymon,

The commented out test that you're referring to, is an existing test (not
that I added or commented). I was going to remove but interestingly its
testing a part of code where (prima-facie) it should fail, but it passes
(probably why it was disabled in the first place)
!

So technically I hope this regression patch I submitted could go through
since this feedback isn't towards that patch, but in my part I am quite
intrigued about this test (and how it passes) and probably I'd get back on
this thread about this particular commented out test in question, as time
permits.

--
Robins Tharakan

On 25 June 2013 04:12, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:

> Thanks a ton Szymon (for a reminder on this one).
>
> As a coincidental turn of events, I have had to travel half way across the
> world and am without my personal laptop (without a linux distro etc.) and
> just recovering from a jet-lag now.
>
> I'll try to install a VM on a make-shift laptop and get something going to
> respond as soon as is possible.
>
> Thanks
> --
> Robins Tharakan
>
> --
> Robins Tharakan
>
>
> On 17 June 2013 05:19, Szymon Guz <mabewlun(at)gmail(dot)com> wrote:
>
>> On 23 May 2013 00:34, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:
>>
>>> Hi,
>>>
>>> Please find attached a patch to take code-coverage of CREATE OPERATOR
>>> (src/backend/commands/operatorcmds.c) from 56% to 91%.
>>>
>>> Any and all feedback is welcome.
>>> --
>>> Robins Tharakan
>>>
>>>
>>> --
>>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgsql-hackers
>>>
>>>
>> Hi,
>> there is one commented out test. I think it should be run, or deleted.
>> There is no use of commented sql code which is not run.
>>
>> What do you think?
>>
>> regards,
>> Szymon
>>
>
>

OK, so I think this patch can be committed, I will change the status.

thanks,
Szymon

On 26 June 2013 09:26, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:

> Hi Szymon,
>
> The commented out test that you're referring to, is an existing test (not
> that I added or commented). I was going to remove but interestingly its
> testing a part of code where (prima-facie) it should fail, but it passes
> (probably why it was disabled in the first place)
> !
>
>
> So technically I hope this regression patch I submitted could go through
> since this feedback isn't towards that patch, but in my part I am quite
> intrigued about this test (and how it passes) and probably I'd get back on
> this thread about this particular commented out test in question, as time
> permits.
>
> --
> Robins Tharakan
>
>
> On 25 June 2013 04:12, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:
>
>> Thanks a ton Szymon (for a reminder on this one).
>>
>> As a coincidental turn of events, I have had to travel half way across
>> the world and am without my personal laptop (without a linux distro etc.)
>> and just recovering from a jet-lag now.
>>
>> I'll try to install a VM on a make-shift laptop and get something going
>> to respond as soon as is possible.
>>
>> Thanks
>> --
>> Robins Tharakan
>>
>> --
>> Robins Tharakan
>>
>>
>> On 17 June 2013 05:19, Szymon Guz <mabewlun(at)gmail(dot)com> wrote:
>>
>>> On 23 May 2013 00:34, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:
>>>
>>>> Hi,
>>>>
>>>> Please find attached a patch to take code-coverage of CREATE OPERATOR
>>>> (src/backend/commands/operatorcmds.c) from 56% to 91%.
>>>>
>>>> Any and all feedback is welcome.
>>>> --
>>>> Robins Tharakan
>>>>
>>>>
>>>> --
>>>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>>>> To make changes to your subscription:
>>>> http://www.postgresql.org/mailpref/pgsql-hackers
>>>>
>>>>
>>> Hi,
>>> there is one commented out test. I think it should be run, or deleted.
>>> There is no use of commented sql code which is not run.
>>>
>>> What do you think?
>>>
>>> regards,
>>> Szymon
>>>
>>
>>
>

On 06/26/2013 12:29 AM, Szymon Guz wrote:
> OK, so I think this patch can be committed, I will change the status.

Can we have a full review before you mark it "ready for committer"? How
did you test it? What kinds of review have you done?

The committer can't know whether it's ready or not if he doesn't have a
full report from you.

Thanks!

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 26 June 2013 20:55, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> On 06/26/2013 12:29 AM, Szymon Guz wrote:
> > OK, so I think this patch can be committed, I will change the status.
>
> Can we have a full review before you mark it "ready for committer"? How
> did you test it? What kinds of review have you done?
>
> The committer can't know whether it's ready or not if he doesn't have a
> full report from you.
>
> Thanks!
>
>
Hi Josh,
I will add more detailed descriptions to all patches I set as read for
committer.

Szymon

On 26 June 2013 20:57, Szymon Guz <mabewlun(at)gmail(dot)com> wrote:

> On 26 June 2013 20:55, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> On 06/26/2013 12:29 AM, Szymon Guz wrote:
>> > OK, so I think this patch can be committed, I will change the status.
>>
>> Can we have a full review before you mark it "ready for committer"? How
>> did you test it? What kinds of review have you done?
>>
>> The committer can't know whether it's ready or not if he doesn't have a
>> full report from you.
>>
>> Thanks!
>>
>>
>
>
Hi Josh,
so I've got a couple of questions.

Is it enough to provide the description in the commitfest app, or is that
better to send an email and provide link in commitfest?

This is a patch only with regression tests, is that enough to write
something like: "This patch applies cleanly on trunk code. All tests pass,
the test coverage increses as provided."? Or do you expect some more info?

thanks,
Szymon

> Is it enough to provide the description in the commitfest app, or is that
> better to send an email and provide link in commitfest?

Better to do it here, on the list.

> This is a patch only with regression tests, is that enough to write
> something like: "This patch applies cleanly on trunk code. All tests pass,
> the test coverage increses as provided."? Or do you expect some more info?

Yes, mainly:

a) does it test what it purports to test?

b) do the tests pass on your machine?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 26 June 2013 21:10, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

>
> > Is it enough to provide the description in the commitfest app, or is that
> > better to send an email and provide link in commitfest?
>
> Better to do it here, on the list.
>
> > This is a patch only with regression tests, is that enough to write
> > something like: "This patch applies cleanly on trunk code. All tests
> pass,
> > the test coverage increses as provided."? Or do you expect some more
> info?
>
> Yes, mainly:
>
> a) does it test what it purports to test?
>
> b) do the tests pass on your machine?
>
>
Done, could you confirm that it is OK now?
I've also checked all the patches on the newest trunk.

thanks,
Szymon

On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz <mabewlun(at)gmail(dot)com> wrote:
> OK, so I think this patch can be committed, I will change the status.

We have a convention that roles created by the regression tests needs
to have "regress" or something of the sort in the name, and that they
need to be dropped by the regression tests. The idea is that if
someone runs "make installcheck" against an installed server, it
should pass - even if you run it twice in succession. And also, it
shouldn't be likely to try to create (and then drop!) a role name that
already exists.

Setting this to "Waiting on Author".

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sure Robert.
I 'll update the tests and get back.

Two questions, while we're at it:

1. Any other conventions (for naming)?
2. Should I assume that all database objects that get created, need to be
dropped explicitly? Or is this point specifically about ROLES?

--
Robins Tharakan

On 27 June 2013 09:00, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz <mabewlun(at)gmail(dot)com> wrote:
> > OK, so I think this patch can be committed, I will change the status.
>
> We have a convention that roles created by the regression tests needs
> to have "regress" or something of the sort in the name, and that they
> need to be dropped by the regression tests. The idea is that if
> someone runs "make installcheck" against an installed server, it
> should pass - even if you run it twice in succession. And also, it
> shouldn't be likely to try to create (and then drop!) a role name that
> already exists.
>
> Setting this to "Waiting on Author".
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Robins Tharakan <tharakan(at)gmail(dot)com> writes:
> 2. Should I assume that all database objects that get created, need to be
> dropped explicitly? Or is this point specifically about ROLES?

It's about any global objects (that wouldn't get dropped by dropping the
regression database). As far as local objects go, there are benefits to
leaving them around, particularly if they present interesting test cases
for pg_dump/pg_restore.

regards, tom lane

On 27 June 2013 09:00, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz <mabewlun(at)gmail(dot)com> wrote:
> > OK, so I think this patch can be committed, I will change the status.
>
> We have a convention that roles created by the regression tests needs
> to have "regress" or something of the sort in the name, and that they
> need to be dropped by the regression tests. The idea is that if
> someone runs "make installcheck" against an installed server, it
> should pass - even if you run it twice in succession. And also, it
> shouldn't be likely to try to create (and then drop!) a role name that
> already exists.
>
> Setting this to "Waiting on Author".
>
> Hi Robert,

Attached is an updated patch that prepends 'regress' before role names.

As for dropping ROLEs is concerned, all the roles created in the previous
patch were within transactions. So didn't have to explicitly drop any ROLEs
at the end of the script.
--
Robins Tharakan

On 26 June 2013 02:26, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:

> So technically I hope this regression patch I submitted could go through
> since this feedback isn't towards that patch, but in my part I am quite
> intrigued about this test (and how it passes) and probably I'd get back on
> this thread about this particular commented out test in question, as time
> permits.
>
>
Attached is an updated (cumulative) patch, that takes care of the issue
mentioned above and tests two more cases that were skipped earlier.

--
Robins Tharakan