Ticket #3708 (closed defect: fixed)

Opened 12 months ago

Last modified 10 months ago

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

Reported by: mooffie Owned by: zaytsev
Priority: minor Milestone: 4.8.19
Component: tests Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

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.

Change History

comment:1 follow-up: ↓ 2 Changed 12 months ago by 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.

Changed 12 months ago by mooffie

comment:2 in reply to: ↑ 1 Changed 12 months ago by 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.

comment:3 Changed 12 months ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.19

Branch: 3708_tests_usability
Initial changeset: 7bfddbee743445bda4e0203960f8b40691396940

comment:4 Changed 12 months ago by zaytsev

Awesome, thank you! Love it...

comment:5 follow-up: ↓ 6 Changed 12 months ago by 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.

comment:6 in reply to: ↑ 5 Changed 12 months ago by 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.))

comment:7 follow-up: ↓ 8 Changed 12 months ago by 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...

comment:8 in reply to: ↑ 7 Changed 12 months ago by 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.))

comment:9 Changed 12 months ago by 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.

comment:10 Changed 12 months ago by zaytsev

  • Branch state changed from on review to on rework

comment:11 Changed 10 months ago by zaytsev

  • Branch state changed from on rework to on review

Branch: 3708_tests_usability
Initial changeset: b5cc581744677921ad6d78d176243d148820566f

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

comment:12 follow-up: ↓ 13 Changed 10 months ago by mooffie

  • Branch state changed from on review to on rework

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.

comment:13 in reply to: ↑ 12 Changed 10 months ago by 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.)

comment:14 Changed 10 months ago by 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)

comment:15 Changed 10 months ago by mooffie

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

comment:16 Changed 10 months ago by zaytsev

  • Branch state changed from on rework to on review

Branch: 3708_tests_usability
Initial changeset: 9d74b18afa37b3e9eeae4ad2c1fbfce9a8f2907c

Third attempt ;-)

comment:17 Changed 10 months ago by andrew_b

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

comment:18 Changed 10 months ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:19 Changed 10 months ago by zaytsev

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [67b3d6495797c2ef7724a9b403836cb7ffb7b9cf]

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

comment:20 Changed 10 months ago by zaytsev

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.