Code review comment for lp:~dandrader/geis/lp984069

Revision history for this message
Stephen M. Webb (bregma) wrote :

(1) m4/xorg-gtest.m4 is actually a convenience copy of a source file belonging to another package and will likely be overwritten at build time. I would suggest the CHECK_GTEST macro be hoisted into its own .m4 file so it doesn't get trampled: it's already renamed so there shouldn't be a conflict.

(2) Not sure why you did all the fancy wrangling in testsuite/gtest/Makefile.am to provide multiple libraries instead of just including the xorg-gtest sources conditionally on ENABLE_INTEGRATION_TESTS. Could you make your reasoning explicit?

(3) Why reorganize testsuite/geis2/Makefile.am? Traditionally all the build targets go at the top (eg. *_PROGRAMS) and the detailed rules for building them come later, rather than the other way around. Granted, that tradition started because automake required it, but following convention makes it easier from someone who sees a lot of Makefile.ams to follow unfamiliar code. Could you make your reasoning for this explicit?

(4) If you're changing testsuite libraries around anyway, would it be possible to consolidate all support libraries used in the testsuite (gtest, mocks) into a single subdirectory? It would simplify the Makefile.ams used in the various test suites. Feel free to argue this.

review: Needs Fixing

« Back to merge proposal