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

[Bryce W. Harrington]

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.

Bryce

> >   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.