Thread: 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
Attachment
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
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-hackersHi,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)
--
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 TharakanOn 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-hackersHi,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@gmail.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 TharakanOn 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 TharakanOn 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-hackersHi,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@agliodbs.com> wrote:
On 06/26/2013 12:29 AM, Szymon Guz wrote:Can we have a full review before you mark it "ready for committer"? How
> OK, so I think this patch can be committed, I will change the status.
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@gmail.com> wrote:
On 26 June 2013 20:55, Josh Berkus <josh@agliodbs.com> wrote:On 06/26/2013 12:29 AM, Szymon Guz wrote:Can we have a full review before you mark it "ready for committer"? How
> OK, so I think this patch can be committed, I will change the status.
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@agliodbs.com> wrote:
Better to do it here, on the list.
> Is it enough to provide the description in the commitfest app, or is that
> better to send an email and provide link in commitfest?Yes, mainly:
> 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?
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@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
Sure Robert.
I 'll update the tests and get back.
Two questions, while we're at it:
1. Any other conventions (for naming)?
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@gmail.com> wrote:
On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz <mabewlun@gmail.com> wrote:We have a convention that roles created by the regression tests needs
> OK, so I think this patch can be committed, I will change the status.
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@gmail.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@gmail.com> wrote:
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.
On Wed, Jun 26, 2013 at 3:29 AM, Szymon Guz <mabewlun@gmail.com> wrote:We have a convention that roles created by the regression tests needs
> OK, so I think this patch can be committed, I will change the status.
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
Attachment
On 26 June 2013 02:26, Robins Tharakan <tharakan@gmail.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