Usability problems with '--enable-tests' (configure.ac)

[mooffie]

To enable tests, one has to pass --enable-tests to configure. But there are a few usability problems in this mechanism:

When the programmer doesn't pass --enable-tests:

In this case, the output on screen and the generated files falsely give the impression tests were enabled:

(1) The message "checking for CHECK..." appears.

(2) The makefiles under the 'tests' tree are generated, even though configure.ac has a conditional that's supposed to prevent this (i.e., if test x$enable_tests != xno).

When the programmer does pass --enable-tests:

(3) She doesn't have an easy way to know if tests were indeed successfully enabled. That's because error messages are very hard to spot, and because the test makefiles, because of (2), are created unconditionally and therefore can't serve as indicator.

Additionally:

(4) Instructions on how to enable the tests should appear where they're expected.

---

Are these usability problems grave?

Yes! People are ​wasting time trying to figure out these issues by themselves.

---

The attached patch fixes these problems:

• Problems (1) and (2) are caused by comparing $enable_tests with "no". But $enable_tests usually contains empty string in this case, not "no". We fix this.

• We solve problem (3) by adding some text to the final summary message.

• We solve problem (4) by adding a README file, in the place where it's expected.

[andrew_b]

I think, tests/README should be added to the EXTRA_DIST in tests/Makefile.am, otherwise it will not be in distributed tarboll.

[mooffie]

Replying to andrew_b:

I think, tests/README should be added to the EXTRA_DIST in tests/Makefile.am, otherwise it will not be in distributed tarboll.

Right. Fixed.

[zaytsev]

Branch: 3708_tests_usability
Initial changeset: 7bfddbee743445bda4e0203960f8b40691396940

[zaytsev]

Awesome, thank you! Love it...

[zaytsev]

Hmmm, Travis doesn't like it:

​https://travis-ci.org/MidnightCommander/mc/builds/172262523

It seems that make dist is botched, need to see why; that's why I'm making so much fuss about CI: problem caught early before causing any damage.

[mooffie]

Replying to zaytsev:

Hmmm, Travis doesn't like it:

​https://travis-ci.org/MidnightCommander/mc/builds/172262523

/bin/bash: line 10: cd: tests: No such file or directory
make: *** [distdir] Error 1

It seems that make dist is botched

I see.

So it turns out that makefiles can't be created conditionally, because they always have to be there to tell make how to create the distribution.

This means that --disable-tests would have triggered the problem too.

I'm attaching v2 of the patch. It removes the conditional from configure.ac.

(I haven't yet had the time to test this patch; it's quite tortuous to run 'make whatever' on my system (which is why I refrained from trying out 'make dist' earlier.))

[zaytsev]

(I haven't yet had the time to test this patch; it's quite tortuous to run 'make whatever' on my system (which is why I refrained from trying out 'make dist' earlier.))

We've talked about that in the past and the obvious solution was rejected, but here are a few more (hopefully acceptable) suggestions:

• You can use the Travis integration that I have developed to offload exhaustive testing to their VMs, just fork the repository to your account and enable Travis for it; you will get notifications from Travis if stuff you push to it doesn't work. (Careful about master branch builds, they will try to deploy source indices which will fail for your account.)

• A long time ago, mc was regularly built with tcc; it is quite possible that this no longer works, but if you manage to get it going, I wouldn't be surprised about an x10 speedup.

• I think I've mentioned ccache already at some point...

[mooffie]

Ok, I've verified that 'make dist' works. Which is not surprising now as the patch no longer contains fancy stuff: it contributes just a "Unit tests: X" summary line and a README. Well, it's better than the current situation.

(@Yury: Thanks. I'll have a look at ccache soon (heard of it before but I guess I was sceptical). Then tcc. (I've already tried memory filesystem and clang, but they weren't much an improvement.))

[zaytsev]

I've already tried memory filesystem and clang, but they weren't much an improvement.

Yes, for a project like mc, tmpfs + clang won't bring much, but even with large C++ projects, clang is not that much faster than gcc these days. I would expect tmpfs + tcc to really rock though, if it works at all.

I'll have a look at ccache soon (heard of it before but I guess I was sceptical).

ccache is very nice as long as you don't often change stuff that ripples all over the code base, such that effectively you'll have to recompile everything anyways, and thus it will be of little help; it all depends on your workflow. In the case of mc, the config.h / version.h thing will, unfortunately, effectively kill the caching upon new commits, but since my compile times are very low, I never cared enough to disentangle this stuff and put version strings in a separate object file.

[zaytsev]

Branch: 3708_tests_usability
Initial changeset: b5cc581744677921ad6d78d176243d148820566f

Rebased the new patch against latest master, Travis build seems to pass now.

[mooffie]

Folks,

At some moment in the past two months I got a jolt of Psychic Insight. It lasted for only a second but as it was flowing through me it occured to me that the intention of the original author of mc-tests.m4 was for '--enable-tests' to effectively be the default.

This makes a thing or two seem logical now.

So, seen in this new light, the patch needs work. I'll do this sometime soon.

[mooffie]

Replying to mooffie:

So, seen in this new light, the patch needs work.

(First, of course, checking whether the current configuration works as intended.)

[zaytsev]

I'm not quite sure this was Slava's intention, otherwise he would have added action-if-given / action-if-not-given as he did to many other similar options, but I would definitively approve of a plan to enable them by default if check is found. Whatever makes tests easier to build and run is a good thing (tm)

[mooffie]

Ok, here's the revised patch. Tests are now enabled by default. No reason not to.

[zaytsev]

Branch: 3708_tests_usability
Initial changeset: 9d74b18afa37b3e9eeae4ad2c1fbfce9a8f2907c

Third attempt ;-)

[andrew_b]

Please add "Ticket #3708:" to the commit message.

[zaytsev]

Merged to master: [67b3d6495797c2ef7724a9b403836cb7ffb7b9cf]

Sorry, Andrew, I totally forgot about this, thanks for the reminder.