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