[PATCH weston 1/5] tests: always build tests

[Peter Hutterer]

On Wed, Sep 11, 2013 at 02:38:55PM +0200, sardemff7+wayland at 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 sure way to turn off
developers...

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

fwiw, I don't have a "packager's POV", I consider myself a developer.
Packaging is just part of the job, but I (currently) do none of that
for wayland-related packages.

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

again, I am aware building them by default won't make everyone suddenly care
about tests. it won't make everyone run the tests. it won't get rid of the
"if it builds, it works" mentality. 

what it does is elevate the tests to something that every developer has to
care about to at least not break. from there, we can then hope that if
they're fixing a test to build again, maybe they're also aware enough to
actually run the test.

And last: git does not have good support for running tests.  I can
auto-run make check before pushing, I can auto-run it after pulling, etc.
But if you don't have a pull-based development model, nothing forces you to
run make check before sending out patches. so you will see patches being
sent where people didn't run make check. And depending on the test suite,
there won't be a punishment, because how are you going to tell? 
if the test suite covers only 10% of the code and you work in some unrelated
part that isn't covered at all, make check is just noise. and humans have a
tendency to drop things that are noise.

Cheers,
   Peter