Re: [PATCH weston 1/5] tests: always build tests
On Wed, Sep 11, 2013 at 02:38:55PM +0200, sardemff7+wayl...@sardemff7.net wrote: > On 11/09/2013 11:12, Sam Spilsbury wrote: > >Quick thought: there's also an important psychological effect to > >building the tests on a standard make because it promotes them to the > >same importance as the rest of your code. They become less of an > >afterthought and it promotes greater care around how people design > >the tests (eg, making the tests clean, making sure they run > >quickly), as well as how the rest of the codebase interacts with the > >tests. We observed a similar effect at Canonical between the projects > >which had test building on by default as opposed to those that did > >not. > > Then we should definitely fix users (developers) and their workflow, > not some arbitrary “problem”, as I said already. arbitrary: "Based on random choice or personal whim, rather than any reason or system." libevdev, libwacom, xserver are three projects I know from the top of my head that use this approach, and it has paid off, at last to some degree. xf86-input-wacom doesn't, and right now the tests won't build. it's not an arbitrary 'problem'. It has a distinct set of reasons, even if you can just attribute it to developer lazyness. > >It all depends on whether or not the tests are there as a basic > >safety line for managing releases or whether or not tests are used > >as a tool to iterate and improve quality. In the latter case, > >building them by default is a very sensible decision indeed. > > Not at all. They should be *run* by default in this case, not just be > built. If their point is to check the code, they must do that, not just > build against some headers. See the end of this email. these are not mutually exclusive, and no-one has claimed that because they are building by default means we don't have to run them. Sam (and I) argue that if you always build them you're increasing the likelyhood of someone actually running them. > On Wed, Sep 11, 2013 at 4:53 PM, Peter Hutterer wrote: > >from my experience, every project I've worked on that has a test > >suite needed this patch eventually, there's always a way to forget > >to run make check and suddenly you find out that it's been broken > >for months. (this is largely because test suites have a tendency to > >become outdated and useless, but...) > > Again, users need a “patch”. I’ll add a bit on that at the end of this > email. > > > >how so? TESTS defines what's run during make check. check_* defines > >what's built, the two are related but not the same. > > Because building tests means you need their *dependencies*, which should > definitely not be needed if you do not want to run them. then make those dependendencies optional? e.g. libevdev will simply print a warning if you don't have check installed at configure time. you can build it, but you won't be able to build (and thus run) the tests. > >I know the principle, I'm claiming that without this, tests will > >eventually break unless you find a way to force everyone to run make > >check. > > > >Which, coincidentally, wastes maintainer time too if you get a patch > >that's fine but breaks make check and you have to get through another > >revision. > > So, you have tests that build. Fine. Now what? If nobody run them, > they are useless. The best way to force them is the vcs. Just add a > git hook that runs them on commit or push, are you are sure you repo > will never break. But you should teach people to run them, not force > them to do so. again, not mutually exclusive. you can force people to build them, and you should still teach people to run them. having tests fail the build merely elevates their presence. > A good example (for me) is Cairo. Their tests are noinst_* already > (side note: we have to patch that in Exherbo to avoid circular > dependencies on fresh installs). > They are known to be broken for ages, but they build fine, sure! not building them by default would have changed this how? at last, if someone now comes along and wants to fix them, they don't have to trudge through months or years of API changes but can focus on the actual test part. > Having tests built by default is not a bad idea, it is a bad fix. > The developer *will* think “Hey, tests build, everything’s ok!” > while his commit just broke them. "if it builds it works" is a mindset that no amount of tests will be able to fix, and, quite frankly, I'd worry more about this mindset in the main compositor code than in some tests that don't cover much code anyway (atm). > If you want to force tests build or run, you can tweak rules a bit: > > if RUN_TESTS_BY_DEFAULT > ifneq ($(ALREADY_INSIDE),yes) > all-local: > $(MAKE) ALREADY_INSIDE=yes check > endif > else > if BUILD_TESTS_BY_DEFAULT > ifneq ($(ALREADY_INSIDE),yes) > all-local: > $(MAKE) TESTS= ALREADY_INSIDE=yes check > endif > endif > endif > > It is both more explicit and saner, imo. running make check after every compile is a
Re: [PATCH weston 1/5] tests: always build tests
On Wed, Sep 11, 2013 at 02:38:55PM +0200, sardemff7+wayl...@sardemff7.net wrote: > On 11/09/2013 11:12, Sam Spilsbury wrote: > >Quick thought: there's also an important psychological effect to > >building the tests on a standard make because it promotes them to the > >same importance as the rest of your code. They become less of an > >afterthought and it promotes greater care around how people design > >the tests (eg, making the tests clean, making sure they run > >quickly), as well as how the rest of the codebase interacts with the > >tests. We observed a similar effect at Canonical between the projects > >which had test building on by default as opposed to those that did > >not. > > Then we should definitely fix users (developers) and their workflow, > not some arbitrary “problem”, as I said already. > > > >It all depends on whether or not the tests are there as a basic > >safety line for managing releases or whether or not tests are used > >as a tool to iterate and improve quality. In the latter case, > >building them by default is a very sensible decision indeed. > > Not at all. They should be *run* by default in this case, not just be > built. If their point is to check the code, they must do that, not just > build against some headers. See the end of this email. I agree with the sentiment here, but I think Peters patch is a pragmatic step towards that goal. Kristian ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/5] tests: always build tests
On Wed, Sep 11, 2013 at 03:58:07PM +1000, Peter Hutterer wrote: > check_PROGRAMS and friends are only built during make check. Which is a > great way of introducing compiler errors in tests. Always build them, TESTS > defines what's being run during make check. Yay, I like that. I thought you had to use check_* with for make check, but of course it's TESTS that matter. thanks, Kristian > --- > tests/Makefile.am | 12 > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 82bf630..398a275 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -29,18 +29,14 @@ clean-local: > export abs_builddir > > noinst_LTLIBRARIES = \ > - $(weston_test) > + $(weston_test) \ > + $(module_tests) > > noinst_PROGRAMS =\ > $(setbacklight) \ > - matrix-test > - > -check_LTLIBRARIES = \ > - $(module_tests) > - > -check_PROGRAMS = \ > $(shared_tests) \ > - $(weston_tests) > + $(weston_tests) \ > + matrix-test > > AM_CFLAGS = $(GCC_CFLAGS) > AM_CPPFLAGS =\ > -- > 1.8.3.1 > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/5] tests: always build tests
On Wed, Sep 11, 2013 at 8:38 PM, wrote: > On 11/09/2013 11:12, Sam Spilsbury wrote: >> >> Quick thought: there's also an important psychological effect to >> building the tests on a standard make because it promotes them to the >> same importance as the rest of your code. They become less of an >> afterthought and it promotes greater care around how people design >> the tests (eg, making the tests clean, making sure they run >> quickly), as well as how the rest of the codebase interacts with the >> tests. We observed a similar effect at Canonical between the projects >> which had test building on by default as opposed to those that did >> not. > > > Then we should definitely fix users (developers) and their workflow, > not some arbitrary “problem”, as I said already. I think we're all in agreement that having maintained tests is a good thing. Generally speaking I've found that most projects which use a test-driven workflow have them *built* by default and then have a separate rule (eg make check or make test) to run them. They also have a CI system which automatically runs the tests and fails patches which fail the tests. I don't know how well CI systems would work for a patches-on-the-mailinglist type workflow, but there are plenty of very well integrated systems for code hosts like launchpad and github. Building tests by default makes them more obvious, especially to newcomers. Running them by default would certainly make them even more obvious. But we don't run them by default because then we would have to accept the constant time process of running the tests every time "make" is invoked (which is about seven seconds in our case). There's also the problem with the weston tests not running headless (although they should). A run of the tests isn't mandatory every time the build completes - its up to the discretion of the developer as to when it would be appropriate to run them. Building them by default at least gives you quick feedback of when you stepped on a test. Building the tests by default is a structural change that promotes positive impacts on workflow and more thought about testing. It isn't an entire solution to the problem of developers not writing tests, but it certainly gets you part of the way there. At least with a testsuite which has no dependencies, there doesn't seem to be a very good reason not to at least build them by default. Best Regards, Sam. -- Sam Spilsbury ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/5] tests: always build tests
On 11/09/2013 11:12, Sam Spilsbury wrote: Quick thought: there's also an important psychological effect to building the tests on a standard make because it promotes them to the same importance as the rest of your code. They become less of an afterthought and it promotes greater care around how people design the tests (eg, making the tests clean, making sure they run quickly), as well as how the rest of the codebase interacts with the tests. We observed a similar effect at Canonical between the projects which had test building on by default as opposed to those that did not. Then we should definitely fix users (developers) and their workflow, not some arbitrary “problem”, as I said already. It all depends on whether or not the tests are there as a basic safety line for managing releases or whether or not tests are used as a tool to iterate and improve quality. In the latter case, building them by default is a very sensible decision indeed. Not at all. They should be *run* by default in this case, not just be built. If their point is to check the code, they must do that, not just build against some headers. See the end of this email. On Wed, Sep 11, 2013 at 4:53 PM, Peter Hutterer wrote: from my experience, every project I've worked on that has a test suite needed this patch eventually, there's always a way to forget to run make check and suddenly you find out that it's been broken for months. (this is largely because test suites have a tendency to become outdated and useless, but...) Again, users need a “patch”. I’ll add a bit on that at the end of this email. how so? TESTS defines what's run during make check. check_* defines what's built, the two are related but not the same. Because building tests means you need their *dependencies*, which should definitely not be needed if you do not want to run them. I know the principle, I'm claiming that without this, tests will eventually break unless you find a way to force everyone to run make check. Which, coincidentally, wastes maintainer time too if you get a patch that's fine but breaks make check and you have to get through another revision. So, you have tests that build. Fine. Now what? If nobody run them, they are useless. The best way to force them is the vcs. Just add a git hook that runs them on commit or push, are you are sure you repo will never break. But you should teach people to run them, not force them to do so. A good example (for me) is Cairo. Their tests are noinst_* already (side note: we have to patch that in Exherbo to avoid circular dependencies on fresh installs). They are known to be broken for ages, but they build fine, sure! Having tests built by default is not a bad idea, it is a bad fix. The developer *will* think “Hey, tests build, everything’s ok!” while his commit just broke them. If you want to force tests build or run, you can tweak rules a bit: if RUN_TESTS_BY_DEFAULT ifneq ($(ALREADY_INSIDE),yes) all-local: $(MAKE) ALREADY_INSIDE=yes check endif else if BUILD_TESTS_BY_DEFAULT ifneq ($(ALREADY_INSIDE),yes) all-local: $(MAKE) TESTS= ALREADY_INSIDE=yes check endif endif endif It is both more explicit and saner, imo. As a last note: if tests do not introduce new dependencies *and* are quick to build, it is fine to build them by default from a packager’s point of view (but not with a check_ vs. noinst_ hack). But please, do not tell me “it will make people keep them up-to-date”. They should be *already*, since people *should* run (and thus build) them. Also, having them broken clearly states “we don’t care about them”, which is sane if upstream really do not care. -- Quentin “Sardem FF7” Glidic ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/5] tests: always build tests
Quick thought: there's also an important psychological effect to building the tests on a standard make because it promotes them to the same importance as the rest of your code. They become less of an afterthought and it promotes greater care around how people design the tests (eg, making the tests clean, making sure they run quickly), as well as how the rest of the codebase interacts with the tests. We observed a similar effect at Canonical between the projects which had test building on by default as opposed to those that did not. It all depends on whether or not the tests are there as a basic safety line for managing releases or whether or not tests are used as a tool to iterate and improve quality. In the latter case, building them by default is a very sensible decision indeed. Regards, Sam. On Wed, Sep 11, 2013 at 4:53 PM, Peter Hutterer wrote: > On 11/09/13 17:32 , sardemff7+wayl...@sardemff7.net wrote: >> >> On 11/09/2013 07:58, Peter Hutterer wrote: >>> >>> check_PROGRAMS and friends are only built during make check. >> >> >> Which is perfectly fine. >> >> >> > Which is a >>> >>> great way of introducing compiler errors in tests. >> >> >> Agree, but we should fix the workflow, not some arbitrary “problem”. > > > from my experience, every project I've worked on that has a test suite > needed this patch eventually, there's always a way to forget to run make > check and suddenly you find out that it's been broken for months. > (this is largely because test suites have a tendency to become outdated and > useless, but...) > > >> > Always build them, TESTS >>> >>> defines what's being run during make check. >> >> >> That’s wrong. > > > how so? TESTS defines what's run during make check. check_* defines what's > built, the two are related but not the same. > > >> The check_* vars are meant this way to avoid forcing >> >> test-only dependencies if you disable tests and to allow one to test her >> code *before* updating the tests. >> Packagers tend to “fix” that the other way around (moving tests from >> noinst_ to check_) quite often… > > > I know the principle, I'm claiming that without this, tests will eventually > break unless you find a way to force everyone to run make check. > > Which, coincidentally, wastes maintainer time too if you get a patch that's > fine but breaks make check and you have to get through another revision. > > Cheers, > Peter > > > >> >>> --- >>> tests/Makefile.am | 12 >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/tests/Makefile.am b/tests/Makefile.am >>> index 82bf630..398a275 100644 >>> --- a/tests/Makefile.am >>> +++ b/tests/Makefile.am >>> @@ -29,18 +29,14 @@ clean-local: >>> export abs_builddir >>> >>> noinst_LTLIBRARIES =\ >>> -$(weston_test) >>> +$(weston_test)\ >>> +$(module_tests) >>> >>> noinst_PROGRAMS =\ >>> $(setbacklight)\ >>> -matrix-test >>> - >>> -check_LTLIBRARIES =\ >>> -$(module_tests) >>> - >>> -check_PROGRAMS =\ >>> $(shared_tests)\ >>> -$(weston_tests) >>> +$(weston_tests)\ >>> +matrix-test >>> >>> AM_CFLAGS = $(GCC_CFLAGS) >>> AM_CPPFLAGS =\ >>> >> >> > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel -- Sam Spilsbury ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/5] tests: always build tests
On 11/09/13 17:32 , sardemff7+wayl...@sardemff7.net wrote: On 11/09/2013 07:58, Peter Hutterer wrote: check_PROGRAMS and friends are only built during make check. Which is perfectly fine. > Which is a great way of introducing compiler errors in tests. Agree, but we should fix the workflow, not some arbitrary “problem”. from my experience, every project I've worked on that has a test suite needed this patch eventually, there's always a way to forget to run make check and suddenly you find out that it's been broken for months. (this is largely because test suites have a tendency to become outdated and useless, but...) > Always build them, TESTS defines what's being run during make check. That’s wrong. how so? TESTS defines what's run during make check. check_* defines what's built, the two are related but not the same. > The check_* vars are meant this way to avoid forcing test-only dependencies if you disable tests and to allow one to test her code *before* updating the tests. Packagers tend to “fix” that the other way around (moving tests from noinst_ to check_) quite often… I know the principle, I'm claiming that without this, tests will eventually break unless you find a way to force everyone to run make check. Which, coincidentally, wastes maintainer time too if you get a patch that's fine but breaks make check and you have to get through another revision. Cheers, Peter --- tests/Makefile.am | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 82bf630..398a275 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -29,18 +29,14 @@ clean-local: export abs_builddir noinst_LTLIBRARIES =\ -$(weston_test) +$(weston_test)\ +$(module_tests) noinst_PROGRAMS =\ $(setbacklight)\ -matrix-test - -check_LTLIBRARIES =\ -$(module_tests) - -check_PROGRAMS =\ $(shared_tests)\ -$(weston_tests) +$(weston_tests)\ +matrix-test AM_CFLAGS = $(GCC_CFLAGS) AM_CPPFLAGS =\ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/5] tests: always build tests
On 11/09/2013 07:58, Peter Hutterer wrote: check_PROGRAMS and friends are only built during make check. Which is perfectly fine. > Which is a great way of introducing compiler errors in tests. Agree, but we should fix the workflow, not some arbitrary “problem”. > Always build them, TESTS defines what's being run during make check. That’s wrong. The check_* vars are meant this way to avoid forcing test-only dependencies if you disable tests and to allow one to test her code *before* updating the tests. Packagers tend to “fix” that the other way around (moving tests from noinst_ to check_) quite often… --- tests/Makefile.am | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 82bf630..398a275 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -29,18 +29,14 @@ clean-local: export abs_builddir noinst_LTLIBRARIES = \ - $(weston_test) + $(weston_test) \ + $(module_tests) noinst_PROGRAMS = \ $(setbacklight) \ - matrix-test - -check_LTLIBRARIES =\ - $(module_tests) - -check_PROGRAMS = \ $(shared_tests) \ - $(weston_tests) + $(weston_tests) \ + matrix-test AM_CFLAGS = $(GCC_CFLAGS) AM_CPPFLAGS = \ -- Quentin “Sardem FF7” Glidic ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
