Thread: Regression tests
Hi, Last weekend I tried to run the regression tests on a different system than what I usually use, but it failed because the system didn't have pg_regress installed. It had a PostgreSQL server running, and it had libpq and libpq header files, but pg_regress and the rest of the PGXS system was not included in those OS packages. It was easy enough to install the right packages (apt-get install ...), but it's nevertheless a bit silly that we depend on PGXS and pg_regress. When I wrote the regression suite, I actually had to jump through some hoops to use pg_regress in the first place. There's the ugly hack where "make installcheck" creates the .sql files that just run the test binaries. Using pg_regress also means that the ODBC.ini file must agree with the current PGHOST etc. settings. Otherwise psql will fail to connect even if odbc.ini is correct, or worse, it will connect to a different server. I've also been trying to make it easier to run the regression tests on Windows. Using psql and pg_regress is a bit of pain there, and again it requires that you have those installed on the build machine, which isn't a given. For these reasons, I propose to refactor the way the regression tests are launched. I wrote a small C utility to basically replace pg_regress. It runs the test programs, compares their outputs with the expected outputs, and runs "diff" to create regression.diffs file if they don't match. It produces TAP-compatible output, which is nice because you can use a frontend like perl 'prove' program to pretty-print it with colors and everything, and you can easily integrate it with tools like Jenkins. But it's also quite human-readable without any such tools. Patch attached. Any objections? The second patch adds a .vcproj file for MSVC, for running the regression tests, and a -RegressionTests option to the BuildAll command to run the regression tests after building the drivers. There are a few regression failures on Windows, so you won't get a clean pass, but at least it's much easier to run the tests with this. - Heikki
Attachment
On Wed, Jan 28, 2015 at 8:42 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> Hi,
>
> Last weekend I tried to run the regression tests on a different system than
> what I usually use, but it failed because the system didn't have pg_regress
> installed. It had a PostgreSQL server running, and it had libpq and libpq
> header files, but pg_regress and the rest of the PGXS system was not
> included in those OS packages.
>
> It was easy enough to install the right packages (apt-get install ...), but
> it's nevertheless a bit silly that we depend on PGXS and pg_regress. When I
> wrote the regression suite, I actually had to jump through some hoops to use
> pg_regress in the first place. There's the ugly hack where "make
> installcheck" creates the .sql files that just run the test binaries.
>
> Using pg_regress also means that the ODBC.ini file must agree with the
> current PGHOST etc. settings. Otherwise psql will fail to connect even if
> odbc.ini is correct, or worse, it will connect to a different server.
>
> I've also been trying to make it easier to run the regression tests on
> Windows. Using psql and pg_regress is a bit of pain there, and again it
> requires that you have those installed on the build machine, which isn't a
> given.
>
> For these reasons, I propose to refactor the way the regression tests are
> launched. I wrote a small C utility to basically replace pg_regress. It runs
> the test programs, compares their outputs with the expected outputs, and
> runs "diff" to create regression.diffs file if they don't match. It produces
> TAP-compatible output, which is nice because you can use a frontend like
> perl 'prove' program to pretty-print it with colors and everything, and you
> can easily integrate it with tools like Jenkins. But it's also quite
> human-readable without any such tools.
>
> Patch attached. Any objections?
No real objections, I am fine with removing the dependency to PGXS.
> The second patch adds a .vcproj file for MSVC, for running the regression
> tests, and a -RegressionTests option to the BuildAll command to run the
> regression tests after building the drivers. There are a few regression
> failures on Windows, so you won't get a clean pass, but at least it's much
> easier to run the tests with this.
Some remarks about the first patch:
1) test/reset-db and test/runsuite are automatically generated. They
should be added in test/.gitignore. On Windows their .exe need to have
entries as well.
2) Tests will fail if test/results does not exist, and that's the case
of a raw tree. You can either create an empty folder test/results with
a .gitignore containing "*", or enforce the creation of this folder if
it does not exist in test/. TBH, the latter is better that having an
empty folder results/ to keep the code tree clean. This problem is of
course the same on all platforms.
3) test/expected/parse.out still has its header line, making the tests fail.
4) There is a typo in this Makefile target:
+runsuit: runsuite.c
5) Shouldn't you use strrchr instead of strchr?
+ /* if the input is a plain test name, construct the binary
name from it */
+ if (strchr(in, DIR_SEP) == NULL)
+ {
+ strcpy(testname, in);
+ sprintf(binname, "src%c%s-test", DIR_SEP, in);
+ return;
+ }
6) I would imagine strrchr here as well:
+ while (*s != '\0' && (s = strchr(s + 1, DIR_SEP)) != NULL)
+ basename = s + 1;
7) s/regression.diff/regression.diffs in runsuite.c
8) It doesn't matter much as process exits quickly, but [coverity]
closing fd in slurpfile() before calling bailout() would be nice
[/coverity].
9) sampletables.sql needs to have 1 SQL per line to have reset-db work
correctly. I don't mind much about this restriction but it seems to me
that this restriction meritates a comment at the top of
sampletables.sql.
About the second patch:
1) winbuild/readme.txt should have a bit of documentation
Regards,
--
Michael
On 01/29/2015 02:36 PM, Michael Paquier wrote:
> Some remarks about the first patch:
> 2) Tests will fail if test/results does not exist, and that's the case
> of a raw tree. You can either create an empty folder test/results with
> a .gitignore containing "*", or enforce the creation of this folder if
> it does not exist in test/. TBH, the latter is better that having an
> empty folder results/ to keep the code tree clean. This problem is of
> course the same on all platforms.
Fixed. I did the latter, because that works when the tests are run from
different directory than where the source is. Hiroshi did some fixing
earlier to make that work (276cb5e4).
> 5) Shouldn't you use strrchr instead of strchr?
> + /* if the input is a plain test name, construct the binary
> name from it */
> + if (strchr(in, DIR_SEP) == NULL)
> + {
> + strcpy(testname, in);
> + sprintf(binname, "src%c%s-test", DIR_SEP, in);
> + return;
> + }
That's just checking if '/' exists, so it doesn't matter.
> 6) I would imagine strrchr here as well:
> + while (*s != '\0' && (s = strchr(s + 1, DIR_SEP)) != NULL)
> + basename = s + 1;
Yeah, here it makes more sense.
> 8) It doesn't matter much as process exits quickly, but [coverity]
> closing fd in slurpfile() before calling bailout() would be nice
> [/coverity].
Nah, seems more trouble than it's worth.
> 9) sampletables.sql needs to have 1 SQL per line to have reset-db work
> correctly. I don't mind much about this restriction but it seems to me
> that this restriction meritates a comment at the top of
> sampletables.sql.
Yes, good point, that should be documented. Added a comment.
Pushed this first patch. Thanks for the review!
- Heikki