Обсуждение: 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@gmail.com> wrote:

On 23 May 2013 00:34, Robins Tharakan <tharakan@gmail.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@postgresql.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@gmail.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@gmail.com> wrote:

On 23 May 2013 00:34, Robins Tharakan <tharakan@gmail.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@postgresql.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!

On 26 June 2013 20:55, Josh Berkus <josh@agliodbs.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!

> 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?

On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz <mabewlun@gmail.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

On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz <mabewlun@gmail.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".

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.