From: | David Steele <david(at)pgmasters(dot)net> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Cc: | Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> |
Subject: | Re: additional contrib test suites |
Date: | 2017-09-14 15:01:06 |
Message-ID: | 0538ed0a-3944-e6c6-f578-43cd338a55cf@pgmasters.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 9/8/17 1:32 PM, Peter Eisentraut wrote:
>
> Yes, some of the error messages had changed. Fixed patches attached.
Patches apply and all tests pass. A few comments:
* [PATCH v2 1/7] adminpack: Add test suite
There are no regular tests for pg_logdir_ls(). It looks like TAP tests
would be required but I'm not sure it's worth it. The fact that the
"default" log name format is hard-coded in is, um, interesting.
Maybe add:
+ SELECT pg_logdir_ls();
+ ERROR: could not read directory "log": No such file or directory
to get a little more coverage? It would be good to at least have a note
on why it is not tested.
* [PATCH v2 4/7] chkpass: Add test suite
Well, this is kind of scary:
+CREATE EXTENSION chkpass;
+WARNING: type input function chkpass_in should not be volatile
I guess the only side effect is that this column cannot be indexed? The
docs say that, so OK, but is there anything else a user should be
worried about?
The rest looks good. I'll mark this "Ready for Committer" since I'm the
only reviewer. I don't think anything you might add based on my
comments above requires a re-review.
As for testing on more platforms, send it to the build farm?
--
-David
david(at)pgmasters(dot)net
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Cramer | 2017-09-14 15:06:25 | Re: SCRAM in the PG 10 release notes |
Previous Message | Amit Khandekar | 2017-09-14 15:00:52 | Re: Parallel Append implementation |