Introduce a tester for extfs helpers

[mooffie]

Here's a tester intended to test the "list" command of extfs helpers.

(Others would use the phrase "test framework" instead of "tester". I'm trying to sound friendly.)

I provide with it tests for uzip, urar, uzoo, and lslR. That's to demonstrate the power of the tester. If this tester gets merged into MC I'll provide tests for more helpers (anybody could, though; it's very easy).

Why

The dire problem of us not currently having tests for covering extfs has been mentioned several times here.

How it works

The tester works by feeding the helpers example input, parsing their output and comparing it to the known correct output.

To understand this better, I ​uploaded the tester here for easier browsing; specifically:

• See the ​README (html format).
• See the ​data directory. The purpose of the '.input' and '.output' files should be self evident even if you don't read the docs.
• As you'll shortly see, each helper needs a very minor modification to make it possible to feed it "fake" input.

How it looks like

(a) Either do "make check"; or:
(b) Run the tester directly, with "run", to see colorful output:

(b1) ...when everything works alright:

(b2) ...when some helper has a bug:

[mooffie]

Some random notes:

1. Yeah, I know the masses are ​clamoring for FISH tests. At the moment I'm not too familiar with the FISH scripts and I want to leave that out of this ticket.

2. A few words about "integration tests" vs "unit tests": There's ​another method to do testing, if MC had scripting support. That method lets us discover many bugs in the VFS, even bugs we didn't intend to cover. But, as Yury ​explains there, those are "integration tests", and as such they have a drawback: they don't give us the direct control "unit tests" give us on the input each component is fed. I.e., in our case we need to feed a helper various inputs, not just the input our system provides, and unit tests are better for this.

[mooffie]

(Since this ticket includes a test for uzoo, it's dependent on #3696.)

[mooffie]

(In #3729) It will be very easy to fix these once we have #3730 in.

[mooffie]

(My web host is down so I copied the files to GitHub Pages and updated the links. The downside is that GitHub doesn't let me set "text/plain" MIME type on all files so you won't be able to browse them online. But it's not a big deal: just look in the patches. I wanted a web hosting just because of ​README.html.)

[andrew_b]

I get the following log:

Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/lslR.1.spaces.input
PASSED.
Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/lslR.2.spaces-iso.input
PASSED.
Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/lslR.3.spaces-iso-noslash.input
PASSED.
Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/urar.v4,v3.input
PASSED.
Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/urar.v5.input
PASSED.
Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/uzip.without-zipinfo--mdy.input

The helper 'uzip' produced no output for this input. Something is wrong.

Make sure this helper supports testability: that it uses $MC_TEST_EXTFS_LIST_CMD.

You may try running the helper yourself with:

  $ MC_TEST_EXTFS_LIST_CMD="mc_xcat /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/uzip.without-zipinfo--mdy.input" \
       /usr/bin/perl -w /home/borodin/work/work.c/mc/mc-3730_extfs_tester/BUILD_ROOT/src/vfs/extfs/helpers/uzip list /dev/null

Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/uzip.without-zipinfo--ymd.input

The helper 'uzip' produced no output for this input. Something is wrong.

Make sure this helper supports testability: that it uses $MC_TEST_EXTFS_LIST_CMD.

You may try running the helper yourself with:

  $ MC_TEST_EXTFS_LIST_CMD="mc_xcat /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/uzip.without-zipinfo--ymd.input" \
       /usr/bin/perl -w /home/borodin/work/work.c/mc/mc-3730_extfs_tester/BUILD_ROOT/src/vfs/extfs/helpers/uzip list /dev/null

Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/uzip.with-zipinfo.input
PASSED.
Testing /home/borodin/work/work.c/mc/mc-3730_extfs_tester/tests/src/extfs-helpers-listcmd/data/uzoo.input
PASSED.

[andrew_b]

Branch: 3730_extfs_tester
Initial changeset:13a805bd79182bbd7d0ef89ba5ac1b359d00c478

[mooffie]

Replying to andrew_b:

I get the following log:

Testing .../data/uzip.without-zipinfo--mdy.input
  The helper 'uzip' produced no output for this input. Something is wrong.
  ...
Testing .../data/uzip.without-zipinfo--ymd.input
  The helper 'uzip' produced no output for this input. Something is wrong.
  ...
Testing .../data/uzip.with-zipinfo.input
PASSED.

Oooh, yes, I made a silly mistake in 'uzip'. Too much shell scripting made me forget Perl has different rules about what's true and what's false. Here's a patch to fix this.

(I didn't discover this bug because on 32-bit Ubuntus (my system) @HAVE_ZIPINFO@ is always zero.)

[zaytsev]

Hi mooffie,

What you've done here is truly amazing, and I was meaning to write something encouraging for a few weeks already, but, very unfortunately, I've been kept more busy than ever due to infrastructure problems with a few other OSS projects which needed to be urgently dealt with before it's too late.

Just a few general comments I can make without having a deep look at your code:

I happened to guide my colleagues to implement a system similar in spirit last year for a "compiler" project (which, crazily enough, compiled business models + data into offers for customers at a certain price, if you are interested), checking that the compiled offers/prices are correct for given inputs. At that time, I suggested to use py.test as a test framework, and write a small helper class, from which the tests could inherit. This gave us tons of things for free, and the most important were:

• Test autodiscovery (I understand you have it)
• Automatic parallel execution with xdist module
• Automatic incremental testing for failed tests
• Outputs in machine readable formats (TAP & JUnit)
• The latter made it possible to use it to make performance test suite later

I understand that probably we don't want to depend on Python (and worse even, non-standard modules for it) for this project even for the test suite, but I would still consider integrating with Autotools [1] test harness if possible instead of writing an own one, or looking at something like BATS [2]. I once had to write a test harness in plain POSIX sh that generated JUnit-compatible output, and I got to hate every bit of it...

[1]: ​https://www.gnu.org/software/automake/manual/html_node/Tests.html#Tests
[2]: ​https://github.com/sstephenson/bats

The latter looks *very* neat if you ask me, but on the con side, it seems it's not very well maintained, which is sad. Not sure if you know better ones, this one is the best I've stumbled upon so far.

In any case, even if you chose to keep your own harness in its entirety, maybe you can still get some inspiration from looking at those, and, at very least, I think it would be great if you could also add TAP output. I don't think that's it's really a lot of work, but one could then integrate it with Jenkins plugins as I did the other time with py.test output, or generate nice HTML reports with TAP-Formatter-HTML @ Travis & push them to a page like sources.m-c.org...

I think one problem that we currently have with the test suite, is that the test reports are buried in the logs and can't be visually inspected in a great way. Check this out for instance: ​http://i.imgur.com/K2WK3.png & the web is full of Jenkins Test Result / TAP / JUnit report screenshots.

Thanks once again for tackling this long standing pain point, and cheers for some great code written for that purpose!

[andrew_b]

Replying to mooffie:

Here's a patch to fix this.

Squashed. Thanks.

I moved the tester from tests/src/extfs-helpers-listcmd into tests/src/vfs/extfs/helpers in order to make tests/ tree consistent with src/ one.

[mooffie]

@Yury: Thanks for the info-laden comment, it's much appreciated. Lots of juicy pointers in it.

Just a quick note, as I have to go (I'll reply some other time(s) as I study further your comment):

1. Instead of doing "TEST = test_uzip test_urar test_lslr test_hp48 ..." (for example) in the Makefile, I did "TEST = test_all" (actually "TEST = run"). It's not because I eschewed integration with Autotools but because the 1st approach, since it requires us to manage more files and register things explicitly, looked, to me, a somewhat substantial bother (one might say it goes against ideas like "zero configuration" and "don't repeat yourself").

2. There were two parts to my work:

(A) Recording the behavior of, and misc knowledge about, the various extfs helpers. This is done in the 'data' dir.

This is the part where our investment goes. And this part is essentially independent of whatever test harness/technique we'll eventually settle on.

(B) Writing the execution engine. It's basically 20 lines of shell script (that are bloated to many more lines because of useful help messages and some such).

This part gives out a "fireworks" impression (colorful output and such). But contrary to this impression, there isn't much invested here. This test_all script that I propose doesn't at all commit us to a certain path. I don't have sentiments about it (except, perhaps, to the "test autodetection" idea it implements), and I don't mind switching it (or augmenting it) with some other tool/technique.

[mooffie]

Replying to andrew_b:

I moved the tester from tests/src/extfs-helpers-listcmd into tests/src/vfs/extfs/helpers in order to make tests/ tree consistent with src/ one.

Sounds good. But note that you named the folder simply "helpers", which doesn't reflect the fact that the tester tests only the 'list' command (and not copyin/copyout, as they're very different in nature (and requirements)). Shouldn't we rename it to, say, "helpers-listcmd"?

(Personally, though, I don't mind "helpers". We can always rename it in the future if we add other kinds of tests.)

(@Yury: don't worry, I'm not ignoring the things you wrote!)

[zaytsev]

Hi Mooffie,

You ignoring my comments is the least of my fears ;-) in fact, I'd be happy to have it merged as is, because we can always improve on it later, and it already solves a very important problem that has been annoying us for a long time.

Re. 2a) sure, I get it, this is why I said it was amazing work in the first place. Re. 2b) I quickly glanced through your runner and it *felt* like it was many screenfuls of code (which may very well be an inflated impression), some usual custom ANSI coloring thing (which I suspect doesn't check whether it's being run interactively or not), etc., and this led me to wondering whether we could re-use more stuff from Autotools to keep it slimmer. In any case, as I said, my concerns with custom harnesses is that they usually don't implement any kind of machine parseable output (simplest of all being TAP), and they quickly grow out of control as one starts adding more fancy stuff like parallel execution, incremental testing, etc.

Re. 1), I absolutely agree that test autodiscovery is awesome, but I was thinking in terms of using a configure substitution to provide the list of tests to Autotools automatically, instead of hiding all the tests from its harness behind one single custom runner. I understand that the annoying downside is that you'd have to reconfigure if you plug in a new test (even though this can be automated I guess?), but the advantage is that you can use Autotools parallel test harness, TAP support, etc.

It would be great if you could look into this approach before we dismiss it, even if you'll conclude that we should like to keep the current system as you proposed it, and extend it as needed.

To try to clarify once again my line of thinking, I think that currently one very annoying thing about our test suite is that the output is not grokable. Imagine we had an awesome dashboard summary after running make check, that would have made assessing the situation much easier. To generate such a dashboard we need granular machine-parseable output, like TAP or JUnit.

The unit test framework we use, check, can do it, but we are not taking advantage of it. There is a TAP driver for Autotools as well (same here). Now, if you throw in more custom harnesses, that's not making it any easier. Now, there is stuff like the blood chilling FISH scripts, and if we ever come up with something to test those, this will be yet another harness, etc.

Cheers!

[zaytsev]

Btw, finally got to push the patches for #2707 that I have cleaned up some time ago. Unfortunately, it's listing function doesn't seem to be easily amenable to testing. Another data point to look at... in the mean time, maybe we should merge #2707 in anyways.

[mooffie]

Replying to zaytsev:

in fact, I'd be happy to have it merged as is, because we can always improve on it later

Yes, I strongly agree.

If I change the tester now it will need to undergo a review process that will take time. It's much more urgent to have these tests in place already. The tester is good enough to be committed as-is.

Re. 1), I absolutely agree that test autodiscovery is awesome, but I was thinking in terms of using a configure substitution to provide the list of tests to Autotools automatically, instead of hiding all the tests from its harness behind one single custom runner. [...]

It would be great if you could look into this approach before we dismiss it

I haven't dismissed it. It's just that I couldn't yet come up with a way to auto-configure this stuff.

In fact, I wouldn't mind getting rid of this autodiscovery feature if the price was adding just two or three words per test to the Makefile. I'll be looking into this possibility too.

I think it would be great if you could also add TAP output.

I'm already studying this topic. (Although this won't be needed if I solve the previous issue.)

some usual custom ANSI coloring thing (which I suspect doesn't check whether it's being run interactively or not)

(That's the purpose of [ -t 1 ]. And I've since learned about the ​tput method of doing things. But this stuff will probably go away, which is why this is inside parentheses.)

[mooffie]

@Andrew: I just need to know if you want to settle on "helpers" for the directory name, or want to change it (let me know the new name). I need this information so I can post the patches I have for #3729.

[andrew_b]

Replying to mooffie:

@Andrew: I just need to know if you want to settle on "helpers" for the directory name, or want to change it (let me know the new name).

I renamed "helpers" to "helpers-list".

[andrew_b]

Merged to master: [3a25b7198f31a83dce32d382a323b748e7e2be4e].

git log --pretty=oneline 4b28a7e..3a25b71

[zaytsev-work]

@mooffie, could you please have a look at the build logs for master? The new tests spew out warnings, and in particular, they don't like that rar is not installed

/home/travis/build/MidnightCommander/mc/distrib/mc-4.8.18-127-g455316b/build-all-disabled/src/vfs/extfs/helpers/urar: 1: /home/travis/build/MidnightCommander/mc/distrib/mc-4.8.18-127-g455316b/build-all-disabled/src/vfs/extfs/helpers/urar: rar: not found

and they are unhappy about zip:

Testing /home/travis/build/MidnightCommander/mc/distrib/mc-4.8.18-127-g455316b/build-all-disabled/../tests/src/vfs/extfs/helpers-list/data/uzip.with-zipinfo.input
syntax error at /home/travis/build/MidnightCommander/mc/distrib/mc-4.8.18-127-g455316b/build-all-disabled/src/vfs/extfs/helpers/uzip line 22, near ": ;"
BEGIN not safe after errors--compilation aborted at /home/travis/build/MidnightCommander/mc/distrib/mc-4.8.18-127-g455316b/build-all-disabled/src/vfs/extfs/helpers/uzip line 414.
The helper 'uzip' produced no output for this input. Something is wrong.
Make sure this helper supports testability: that it uses $MC_TEST_EXTFS_LIST_CMD.
You may try running the helper yourself with:
  $ MC_TEST_EXTFS_LIST_CMD="mc_xcat /home/travis/build/MidnightCommander/mc/distrib/mc-4.8.18-127-g455316b/build-all-disabled/../tests/src/vfs/extfs/helpers-list/data/uzip.with-zipinfo.input" \
       /usr/bin/perl -w /home/travis/build/MidnightCommander/mc/distrib/mc-4.8.18-127-g455316b/build-all-disabled/src/vfs/extfs/helpers/uzip list /dev/null

I believe that they shouldn't require the tools to be installed in the build environment, what do you think?

[mooffie]

Replying to zaytsev-work:

syntax error at .../build-all-disabled/src/vfs/extfs/helpers/uzip line 22, near ": ;"

You disabled the extfs VFS, it seems (please confirm). So @HAVE_ZIPINFO@, which is used in 'uzip.in', doesn't get defined by the 'configure' script. This results in invalid Perl syntax in 'uzip'.

This is not a bug.

IIUC, what we need is for "make check" to not run the extfs tester at all if the extfs VFS is disabled. Please confirm my conclusion. I'll provide a patch soon.

they don't like that rar is not installed
.../src/vfs/extfs/helpers/urar: rar: not found

I was aware of that. Line #25 in 'urar' runs rar even when the tester is active. But it doesn't really need to: it's easy to fix this. The reason I haven't done so from the start is because I wanted to show how easy it is to "make a helper testable" and I didn't want to add more than just one or two lines or code. It would have been bad for publicity :-) But now that I no longer need to worry about publicity I'll provide a patch to fix this...

[zaytsev-work]

You disabled the extfs VFS, it seems (please confirm).

Yes, you are right. FYI, you can check how I run tests on Travis here: ​https://github.com/MidnightCommander/mc/blob/master/maint/utils/travis-build.sh#L82

IIUC, what we need is for "make check" to not run the extfs tester at all if the extfs VFS is disabled. Please confirm my conclusion.

I think that this is a reasonable approach: one shouldn't test explicitly disabled components.

But now that I no longer need to worry about publicity I'll provide a patch to fix this...

Please do!

[mooffie]

(Patches were attached. Let me know if all's fine.)

[mooffie]

(I hope my comment:24 wasn't understood incorrectly: having 'urar' run rar under the tester had, I believe, only a cosmetic issue on systems lacking it (but of course I agree that stderr ought to be be kept clean, especially when testing, no need to lecture me). It's not that I knowingly left a bug there to be tripped on later.)

[zaytsev]

But now that I no longer need to worry about publicity...
I hope my comment:24 wasn't understood incorrectly...

I think that you should chill out a little bit and worry less. We are not as heinous of perpetrators as it might appear... :-) Christmas time is coming and all is good.

[andrew_b]

Branch: 3730_extfs_tester_fix
Initial changeset:2c17c7d28ba776bc302227555edca48ea13effc2

[andrew_b]

Merged to master: [f1bc44943c5bdac3f9a2e146bcd0e168423d69a6].

git log --pretty=oneline 455316b..f1bc449

[zaytsev]

@mooffie, also, it now occurred to me, that we still have to list test data in EXTRA_DIST, so maybe one could just as well put test names in a variable, re-arrange them to live in each individual subdirectory, specify them in TESTS and use same variable for EXTRA_DIST.

Or else, simply add the data directory as a whole to EXTRA_DIST. It just feels somehow wrong to me to argue for test autodiscovery to avoid listing test names in TESTS and then still have to register every single test file individually with the build system.

I guess I still like the first idea better, unless we decide that further integration with Autotools harness is not something we want to pursue. Would be interested to hear what do you think about these suggestions...