Thread: pgsql: Add TAP tests for pg_dump
Add TAP tests for pg_dump This TAP test suite will create a new cluster, populate it based on the 'create_sql' values in the '%tests' hash, run all of the runs defined in the '%pgdump_runs' hash, and then for each test in the '%tests' hash, compare each run's output the the regular expression defined for the test under the 'like' and 'unlike' functions, as appropriate. While this test suite covers a fair bit of ground (67% of pg_dump.c and quite a bit of the other files in src/bin/pg_dump), there is still quite a bit which remains to be added to provide better code coverage. Still, this is quite a bit better than we had, and has found a few bugs already (note that the CREATE TRANSFORM test is commented out, as it is currently failing). Idea for using the TAP system from Tom, though all of the code is mine. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/6bd356c33a3cf3a49313dc8638ea4bb066c4cf37 Modified Files -------------- src/bin/pg_dump/Makefile | 3 + src/bin/pg_dump/pg_dump.c | 2 +- src/bin/pg_dump/t/001_basic.pl | 42 + src/bin/pg_dump/t/002_pg_dump.pl | 2859 ++++++++++++++++++++ src/test/modules/Makefile | 1 + src/test/modules/test_pg_dump/.gitignore | 4 + src/test/modules/test_pg_dump/Makefile | 25 + src/test/modules/test_pg_dump/README | 2 + .../modules/test_pg_dump/expected/test_pg_dump.out | 6 + src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 1 + src/test/modules/test_pg_dump/t/001_base.pl | 535 ++++ .../modules/test_pg_dump/test_pg_dump--1.0.sql | 15 + src/test/modules/test_pg_dump/test_pg_dump.control | 3 + 13 files changed, 3497 insertions(+), 1 deletion(-)
* Stephen Frost (sfrost@snowman.net) wrote: > src/test/modules/test_pg_dump/Makefile | 25 + > src/test/modules/test_pg_dump/README | 2 + > .../modules/test_pg_dump/expected/test_pg_dump.out | 6 + > src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 1 + > src/test/modules/test_pg_dump/t/001_base.pl | 535 ++++ > .../modules/test_pg_dump/test_pg_dump--1.0.sql | 15 + > src/test/modules/test_pg_dump/test_pg_dump.control | 3 + Looks like the test_pg_dump extension made the Windows builds upset. I'm guessing that's because I set 'MODULES_big' even though there isn't a .c component. Doing a local build with that commented out, assuming that works and doesn't generate the .so any more on my Linux box, I'll push the change up to hopefully fix those buildfarm members. Thanks! Stephen
Attachment
On 5/6/16 2:06 PM, Stephen Frost wrote: > Add TAP tests for pg_dump I'd be the first to welcome this, but what happened to feature freeze? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > On 5/6/16 2:06 PM, Stephen Frost wrote: > >Add TAP tests for pg_dump > > I'd be the first to welcome this, but what happened to feature freeze? These are just new tests..? I assumed that would be welcome during post feature-freeze, and certainly no one raised any concerns about adding these tests during the discussion prior to my commiting them. We back-patch new tests from time to time too, when they're associated with bug fixes, so I'm pretty confused why TAP tests would be an issue to add on HEAD post feature-freeze. If the consensus is that we shouldn't add new tests during feature freeze, I'll revert the patch that added them and add them later, but, for my 2c at least, I think we should be happy to add these even after feature freeze. Thanks! Stephen
Attachment
On 2016-05-06 15:11:53 -0400, Stephen Frost wrote: > * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > > On 5/6/16 2:06 PM, Stephen Frost wrote: > > >Add TAP tests for pg_dump > > > > I'd be the first to welcome this, but what happened to feature freeze? > > These are just new tests..? I assumed that would be welcome during post > feature-freeze, and certainly no one raised any concerns about adding > these tests during the discussion prior to my commiting them. > > We back-patch new tests from time to time too, when they're associated > with bug fixes, so I'm pretty confused why TAP tests would be an issue > to add on HEAD post feature-freeze. > > If the consensus is that we shouldn't add new tests during feature > freeze, I'll revert the patch that added them and add them later, but, > for my 2c at least, I think we should be happy to add these even after > feature freeze. +1
* Stephen Frost (sfrost@snowman.net) wrote: > * Stephen Frost (sfrost@snowman.net) wrote: > > src/test/modules/test_pg_dump/Makefile | 25 + > > src/test/modules/test_pg_dump/README | 2 + > > .../modules/test_pg_dump/expected/test_pg_dump.out | 6 + > > src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 1 + > > src/test/modules/test_pg_dump/t/001_base.pl | 535 ++++ > > .../modules/test_pg_dump/test_pg_dump--1.0.sql | 15 + > > src/test/modules/test_pg_dump/test_pg_dump.control | 3 + > > Looks like the test_pg_dump extension made the Windows builds upset. > I'm guessing that's because I set 'MODULES_big' even though there isn't > a .c component. > > Doing a local build with that commented out, assuming that works and > doesn't generate the .so any more on my Linux box, I'll push the change > up to hopefully fix those buildfarm members. Alright, apparently that made other Windows buildfarm members unhappy... I guess the next approach will be to add back MODULES_big and add in a .c file for the Windows systems to be happy about. I'm certainly open to other suggestions. The intagg contrib modules doesn't have a .c file either, nor a MODULES_big line, and I don't see any errors with it. I'm not terribly familiar with how the Windows builds are run though. Thanks! Stephen
Attachment
* Stephen Frost (sfrost@snowman.net) wrote: > Alright, apparently that made other Windows buildfarm members unhappy... > > I guess the next approach will be to add back MODULES_big and add in a > .c file for the Windows systems to be happy about. I'm certainly open > to other suggestions. > > The intagg contrib modules doesn't have a .c file either, nor a > MODULES_big line, and I don't see any errors with it. I'm not terribly > familiar with how the Windows builds are run though. It looks like there's an exclude list that intagg is on for the MSVC build animals, presumably because it doesn't have a .c file to be compiled. I'm guessing the right answer here is to just add test_pg_dump to that list. If there aren't any other suggestions, I'll go ahead and do that. Thanks! Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes:
> * Stephen Frost (sfrost@snowman.net) wrote:
>> Looks like the test_pg_dump extension made the Windows builds upset.
>> I'm guessing that's because I set 'MODULES_big' even though there isn't
>> a .c component.
>>
>> Doing a local build with that commented out, assuming that works and
>> doesn't generate the .so any more on my Linux box, I'll push the change
>> up to hopefully fix those buildfarm members.
> Alright, apparently that made other Windows buildfarm members unhappy...
> I guess the next approach will be to add back MODULES_big and add in a
> .c file for the Windows systems to be happy about. I'm certainly open
> to other suggestions.
You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.
But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > * Stephen Frost (sfrost@snowman.net) wrote: > >> Looks like the test_pg_dump extension made the Windows builds upset. > >> I'm guessing that's because I set 'MODULES_big' even though there isn't > >> a .c component. > >> > >> Doing a local build with that commented out, assuming that works and > >> doesn't generate the .so any more on my Linux box, I'll push the change > >> up to hopefully fix those buildfarm members. > > > Alright, apparently that made other Windows buildfarm members unhappy... > > > I guess the next approach will be to add back MODULES_big and add in a > > .c file for the Windows systems to be happy about. I'm certainly open > > to other suggestions. > > You should not need to do that; cf src/test/modules/test_extensions, > which has got SQL-only extensions. test_extensions is also included in the "contrib_excludes" list in src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm thinking that's what is needed. > But at this point I think Peter's complaint has some force to it, and that > what you ought to do is revert the testing patch. You can have another go > after beta1. Are you suggesting commiting to still-9.6-HEAD post-beta1? I took Peter's comment as suggesting that adding the tests would have to wait til after we branched 9.6, as we do for features. I'd really like to have these tests included as that will make them available to others more easily to add on to, and I'm certainly planning to continue adding tests until I get pg_dump.c's coverage a lot better. That seems like the perfect kind of effort that should be happening right now- adding more tests and working to make sure that what's been committed is correct (and fixing it when it isn't, as discovered by the test suite with transforms and casts...). Thanks! Stephen
Attachment
Stephen Frost wrote: > * Stephen Frost (sfrost@snowman.net) wrote: > > The intagg contrib modules doesn't have a .c file either, nor a > > MODULES_big line, and I don't see any errors with it. I'm not terribly > > familiar with how the Windows builds are run though. > > It looks like there's an exclude list that intagg is on for the MSVC > build animals, presumably because it doesn't have a .c file to be > compiled. > > I'm guessing the right answer here is to just add test_pg_dump to that > list. I don't like this solution, because it means pg_dump will get little testing on Windows. I agree with reverting now and figuring out a way to enable this test on Windows after the beta. I suggest you grab hold of your own Windows installation so that you can whack it locally until it works, rather than waiting for buildfarm cycles. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > Stephen Frost wrote: > > * Stephen Frost (sfrost@snowman.net) wrote: > > > > The intagg contrib modules doesn't have a .c file either, nor a > > > MODULES_big line, and I don't see any errors with it. I'm not terribly > > > familiar with how the Windows builds are run though. > > > > It looks like there's an exclude list that intagg is on for the MSVC > > build animals, presumably because it doesn't have a .c file to be > > compiled. > > > > I'm guessing the right answer here is to just add test_pg_dump to that > > list. > > I don't like this solution, because it means pg_dump will get little > testing on Windows. I agree with reverting now and figuring out a way > to enable this test on Windows after the beta. I suggest you grab hold > of your own Windows installation so that you can whack it locally until > it works, rather than waiting for buildfarm cycles. This is only for the test_pg_dump extension, which is specifically just testing pg_dump with extensions. The vast majority of the pg_dump tests are in src/bin/pg_dump and should be getting run on the Windows systems (once this issue is addressed). Thanks! Stephen
Attachment
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > Stephen Frost wrote: >> I'm guessing the right answer here is to just add test_pg_dump to that >> list. > I don't like this solution, because it means pg_dump will get little > testing on Windows. No, that's incorrect: @contrib_excludes stops the MSVC stuff from trying to *build* in that directory, which is fine because there's nothing to build. It doesn't prevent running the test case. See eg. http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=thrips&dt=2016-05-06%2015%3A37%3A27&stg=testmodules-install-check-C where the test_extensions test does get run even though it's in @contrib_excludes. In any case, reverting the patch would mean no Windows testing, so your argument doesn't seem to have much force even if it were true? regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Stephen Frost wrote: > >> I'm guessing the right answer here is to just add test_pg_dump to that > >> list. > > > I don't like this solution, because it means pg_dump will get little > > testing on Windows. > > No, that's incorrect: @contrib_excludes stops the MSVC stuff from trying > to *build* in that directory, which is fine because there's nothing to > build. It doesn't prevent running the test case. See eg. > http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=thrips&dt=2016-05-06%2015%3A37%3A27&stg=testmodules-install-check-C > where the test_extensions test does get run even though it's in > @contrib_excludes. Ah, that's surprising, but much better of course. Works for me. > In any case, reverting the patch would mean no Windows testing, so > your argument doesn't seem to have much force even if it were true? I was suggesting to revert it for the beta and put it back with a working Windows implementation after the beta, before GA. I was afraid that a hack to hide the test from Windows would become permanent. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 5/6/16 4:07 PM, Stephen Frost wrote: > Are you suggesting commiting to still-9.6-HEAD post-beta1? I took > Peter's comment as suggesting that adding the tests would have to wait > til after we branched 9.6, as we do for features. > > I'd really like to have these tests included as that will make them > available to others more easily to add on to, and I'm certainly planning > to continue adding tests until I get pg_dump.c's coverage a lot better. > That seems like the perfect kind of effort that should be happening > right now- adding more tests and working to make sure that what's been > committed is correct (and fixing it when it isn't, as discovered by the > test suite with transforms and casts...). Well, we all want to get our code in and let others make it better. But here we are trying to get a release out in a few days with only a weekend in between, and we are still trying to figure out basic portability issues using build-farm-whacking technology. This is not the time. Again, I applaud this, but this is clearly a new major chunk of mostly not-peer-reviewed code with potential for portability problems and the possibility to break downstream packaging routines. There is no harm in leaving this for 9.7. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 5/6/16 3:11 PM, Stephen Frost wrote: > These are just new tests..? This is a matter of degree, but I think there is a difference between new test cases and a whole new test suite. > I assumed that would be welcome during post > feature-freeze, and certainly no one raised any concerns about adding > these tests during the discussion prior to my commiting them. I didn't see that discussion and I still can't find it. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Again, I applaud this, but this is clearly a new major chunk of mostly
> not-peer-reviewed code with potential for portability problems and the
> possibility to break downstream packaging routines. There is no harm in
> leaving this for 9.7.
FWIW, I was and remain perfectly on board with putting this into 9.6;
adding new tests seems to me to be very much within the charter of beta
test period. My only gripe was that shoving it in so soon before beta1
wrap is risky. But it looks from the current buildfarm results that
Stephen has gotten away with that. If we see any more buildfarm failures
over the weekend, I'd counsel a temporary revert, but if we don't I think
it's OK to ship beta1 with these tests. In either case I'd aim to have
them in place in beta2.
regards, tom lane