Ticket #3730 (closed task: fixed)

Opened 7 years ago

Last modified 7 years ago

Introduce a tester for extfs helpers

Reported by: mooffie Owned by: andrew_b
Priority: major Milestone: 4.8.19
Component: mc-vfs Version: master
Keywords: Cc:
Blocked By: #3696 Blocking: #3729, #3744
Branch state: merged Votes for changeset: committed-master

Description (last modified by mooffie) (diff)

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:

http://i.imgur.com/CivkhnF.png

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

http://i.imgur.com/kjmVK3G.png

Change History

comment:1 Changed 7 years ago by 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.
  1. 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.

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

Changed 7 years ago by mooffie

comment:2 Changed 7 years ago by mooffie

  • Blocked By 3696 added

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

comment:3 Changed 7 years ago by mooffie

  • Blocking 3729 added

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

comment:4 Changed 7 years ago by mooffie

  • Description modified (diff)

comment:5 Changed 7 years ago by mooffie

  • Description modified (diff)

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

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

comment:7 Changed 7 years ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Milestone changed from Future Releases to 4.8.19

Branch: 3730_extfs_tester
Initial changeset:13a805bd79182bbd7d0ef89ba5ac1b359d00c478

comment:8 in reply to: ↑ 6 ; follow-up: ↓ 10 Changed 7 years ago by 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.)

Changed 7 years ago by mooffie

comment:9 Changed 7 years ago by 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!

comment:10 in reply to: ↑ 8 ; follow-up: ↓ 12 Changed 7 years ago by 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.

comment:11 Changed 7 years ago by 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").
  1. 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.

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

comment:13 follow-up: ↓ 15 Changed 7 years ago by 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!

comment:14 Changed 7 years ago by 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.

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

comment:16 follow-up: ↓ 18 Changed 7 years ago by 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.

comment:17 Changed 7 years ago by mooffie

  • Blocking 3744 added

comment:18 in reply to: ↑ 16 Changed 7 years ago by 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".

comment:19 Changed 7 years ago by andrew_b

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

comment:20 Changed 7 years ago by zaytsev-work

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

comment:21 Changed 7 years ago by andrew_b

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

Merged to master: [3a25b7198f31a83dce32d382a323b748e7e2be4e].

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

comment:22 Changed 7 years ago by andrew_b

  • Status changed from testing to closed

comment:23 follow-up: ↓ 24 Changed 7 years ago by 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?

comment:24 in reply to: ↑ 23 Changed 7 years ago by 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...

comment:25 Changed 7 years ago by 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!

comment:26 Changed 7 years ago by mooffie

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

Last edited 7 years ago by mooffie (previous) (diff)

comment:27 Changed 7 years ago by 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.)

comment:28 Changed 7 years ago by 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.

comment:29 Changed 7 years ago by andrew_b

  • Status changed from closed to reopened
  • Votes for changeset committed-master deleted
  • Resolution fixed deleted
  • Branch state changed from merged to no branch

comment:30 Changed 7 years ago by andrew_b

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

Branch: 3730_extfs_tester_fix
Initial changeset:2c17c7d28ba776bc302227555edca48ea13effc2

comment:31 Changed 7 years ago by zaytsev-work

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

comment:32 Changed 7 years ago by andrew_b

  • Status changed from reopened to closed
  • Votes for changeset changed from andrew_b zaytsev to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [f1bc44943c5bdac3f9a2e146bcd0e168423d69a6].

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

comment:33 Changed 7 years ago by 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...

Note: See TracTickets for help on using tickets.