[cairo] [PATCH v3 0/7] test: Don't ignore test output files when written to the wrong directory

Bryce W. Harrington b.harrington at samsung.com
Sun Sep 8 10:46:47 PDT 2013

On Sun, Sep 08, 2013 at 03:03:18PM +0200, Uli Schlachter wrote:
> Hi,
> I started merging this and got up to patch 6/7, then stopped. Some comments below.
> On 15.08.2013 20:08, Bryce W. Harrington wrote:
> [...]
> > Bryce Harrington (7):
> >   test: Don't ignore test output files left in test directory
> >   test: Fix several tests to place output files in the output directory
> >   test: Use CAIRO_TEST_OUTPUT_DIR for name of the output directory
> >   test: concat CAIRO_TEST_OUTPUT_DIR at point of use instead of
> >     BASENAME
> The first patch adds a hardcoded "output/" to some file names, the second one
> changes that into a define and the third one completely undoes the first patch
> and moves the addition of the "output/"-prefix from the define into the code.
> I don't see why this is spread over three different commits. I would just squash
> them together into a single one (which then would be about as large as each of
> the individual commits, so splitting this up doesn't produce smaller patches).
> It just seems like these are fixup commits which correct errors from the earlier
> commit.
> >   test: Make cairo_test_mkdir() usable throughout tests
> Why does this copy&paste _cairo_test_mkdir() to cairo_test_mkdir()? Wouldn't it
> be enough to rename _cairo_test_mkdir() and make it non-static (and adding it to
> cairo-test.h)?

_cairo_test_mkdir() is implemented in indivdual tests.  This commit
places it in the core test code so other tests can link to it.  You
can't just rename one of the existing routine implementations.


> >   test: Ensure output dirs exist, falling back to current dir if needed
> I don't know what I should say. I read the description, then looked at the diff
> and afterward had to read the description again. This is the point were I
> stopped committing the patches, because this makes no sense at all.
> The diff should be squashed into the previous commit and the change described,
> if needed, should be implemented.
> >   test: Refer to output filename by variable, not a hardcoded value
> This could get "test, svg:" instead of just "test". Besides that, dunno. Never
> looked closely at how SVG does things.
> [...]
> Sorry for complaining again (and sorry for being awfully slow for looking at
> things! If you want to, feel free to just do something about patch 6/7 and
> ignore the rest of my complaints, those are not that important).
> Cheers,
> Uli
> -- 
> Sent from my Game Boy.

More information about the cairo mailing list