Convert bin/sh/tests to ATF

[ngie]

Summary

Convert bin/sh/tests to ATF

The new code uses a "test discovery mechanism" to determine
what tests are available for execution

A user-specified test shell can be provided like the following:

kyua test -v test_suites.FreeBSD.bin.sh.test_shell=/path/to/test/sh

Sponsored by: EMC / Isilon Storage Division

[jmmv]

Very nice!

bin/sh/tests/Makefile.inc

3 ↗

(On Diff #970)

This is surprising, to say the least. Where does the right FILESDIR value come from?

Define TESTSDIR explicitly as it is customary and then derive FILESDIR from it, not the other way around. After all, the important thing here are the tests; the files are just supporting data.

7 ↗

(On Diff #970)

All this Makefile.inc does is save two lines in all other Makefiles with the downside that it adds an indirection level and thus makes the logic harder to understand. (For example: when I was reviewing the rest of the diff, I was wondering how KYUAFILE=no was going away but no TESTS definition was being added!)

I'd recommend not adding this Makefile.inc and just defining .PATH and ATF_TESTS_SH in each subdirectory's Makefile instead.

bin/sh/tests/functional_test.sh

30

Don't escape the work directory from test programs, anywhere.

Doing so risks generating garbage elsewhere in the file system that won't be cleaned up by Kyua.

Use atf_get_srcdir and write all paths relative to it.

52

Same thing here regarding the escape of the work directory.

64

This belongs in the _body() function of the test cases, and if you do that, then you probably don't even need to export SH and can keep the value as local.

Minimize the stuff that can go wrong in atf_init_test_cases() so that a failure doesn't cause the whole test program to abort.

[ngie]

bin/sh/tests/Makefile.inc

3 ↗

(On Diff #970)

Done :).

7 ↗

(On Diff #970)

Ok.

bin/sh/tests/functional_test.sh

30

Good point.

[jmmv]

Did you forget to upload a new version of the diff? :-P

[ngie]

• Remove Makefile.inc:
  • Be explicit about the value of TESTSDIR
  • Derive FILESDIR from TESTSDIR
• Change the use of bsd.own.mk to src.opts.mk in builtins/Makefile; remove bsd.own.mk use from all other Makefiles
• Add an entry for legacy_test in OptionalObsoleteFiles.inc
• Don't cd into $(atf_get_srcdir)
• Copy the input files from $(atf_get_srcdir) into the temporary directory instead to ensure that the output is consistent across all runs.

[ngie]

In D547#7, @jmmv wrote:

Did you forget to upload a new version of the diff? :-P

I got tired and passed out at the upload dialog ;)..

[jmmv]

I only have some nits left but this LGTM. jilles should still given an OK though.

bin/sh/tests/builtins/Makefile

5

nit: Because this is only needed by ATF_TESTS_SH, I'd put it right next to the ATF_TESTS_SH definition, without any blank lines in between.

bin/sh/tests/functional_test.sh

32

nit: In general, I like double-quoting all $() and ${} invocations to protect against whitespaces.

58

nit: I think you'd even do:

SH=... atf_check ...

Not that it matters much, but I'm always wary of modifying global state and like narrowing down the scope of side-effects as much as possible.

[ngie]

Apply review comments.

bin/sh/tests/builtins/Makefile

5

Sounds reasonable.

bin/sh/tests/functional_test.sh

32

Good point. I'll do that :)

58

1. The original code exported SH at the top of the script.
2. I thought that ATF executed things in a forked manner -- it could be a problem if the value was inherited across forks and wasn't supposed to be inherited, but given 1., I think we're ok.

[jmmv]

I think you forgot to upload again! But still looks good to me, assuming that things were changed as your previous replies implied ;-)

bin/sh/tests/functional_test.sh

58

Right; each test case is executed in a different process. Basically: create process, exec test program, and tell the test program to run a single body().

So doing the "export SH" from within the body is perfectly fine.

However, the only thing you want to pass the value of SH to is to the ./${tc} file, hence why I said to only override the value of SH when invoking atf_check. But maybe that doesn't work given that atf_check is a shell function and thus may require the export.

[ngie]

Add quotes around $(atf_get_srcdir) (forgot to add this to the last diff).

[ngie]

In D547#16, @jmmv wrote:

I think you forgot to upload again! But still looks good to me, assuming that things were changed as your previous replies implied ;-)

Unfortunately my fallback -- env -- wouldn't work in this case either because it's not a built-in, and thus it wouldn't work with shell functions :/..

[jilles]

The code looks good but a bit strange in a few places. Things like \$([ -f \$se ] && echo \"-e file:\$se\") (in eval "...") cause unnecessary forks and look strange to me (as if you're writing Python in shell); instead, you could set se to the empty string if the file does not exist and expand ${se:+-e file:$se}. I think the eval will look less confusing if most of the code is escaped using single-quotes instead of double-quotes.

More importantly, I'm seeing an approximate 25x slowdown for a kyua test in /usr/tests/bin/sh on my virtualbox VM (from about 6 to about 142 seconds). I have not looked at the internals of kyua yet but could something be done about that?

tests/errors/bad-parm-exp* should probably be changed not to compare the script pathname; this is the second time they need to be adjusted for testsuite reworks. This can be a separate commit though.

[jmmv]

jilles: Regarding the slowdown, it would need some profiling of course. But two guesses:

The first is the change of model. With the switch to ATF, every single test case is now run in its own process, with its own temporary directory and other related "isolation features". These add a penalty, and due to the way Kyua works today, the penalty is not insignificant. (There certainly is room for improvement here and is something I'm planning on tackling soon, but I believe the isolation features are worth it for better determinism of the test cases and reproducibility of failures.)

But note that this is not something that ATF enforces: we'd be defining a single test case within each test program, and having that test case run all of the individual files just as we did before. However, that'd defeat the whole purpose of running test cases individually.

The second is the dynamic nature of the changes being introduced. For example: the find command is being run once for every single test case, and the big chunks of "eval" are probably not super-efficient. Something could be done here as well. I suspect this is a minor penalty compared to what I mention above.

bin/sh/tests/functional_test.sh

55

Didn't notice this earlier.

Why are these files all copied? Just reference them from the source directory where needed below.

59

As jilles said, you'd just do:

stdout_flag=
[ ! -f "${stdout_file}" ] || stdout_flag="-o file:${stdout_file}"
...
atf_check ${stdout_flag}

to avoid the subprocess. (And note the variable names; I think they are much readable and less cryptic than ${so} or ${se}.)

[ngie]

In D547#20, @jilles wrote:

The code looks good but a bit strange in a few places. Things like \$([ -f \$se ] && echo \"-e file:\$se\") (in eval "...") cause unnecessary forks and look strange to me (as if you're writing Python in shell); instead, you could set se to the empty string if the file does not exist and expand ${se:+-e file:$se}. I think the eval will look less confusing if most of the code is escaped using single-quotes instead of double-quotes.

I fixed that by moving things to a check helper function and applying your and Julio's review comments. It's a lot more readable now.

More importantly, I'm seeing an approximate 25x slowdown for a kyua test in /usr/tests/bin/sh on my virtualbox VM (from about 6 to about 142 seconds). I have not looked at the internals of kyua yet but could something be done about that?

My moving the majority of the eval code into a check helper function appears to have helped.

tests/errors/bad-parm-exp* should probably be changed not to compare the script pathname; this is the second time they need to be adjusted for testsuite reworks. This can be a separate commit though.

That's the only reason why the cp exists (I've limited it to just copying the testcase though).

[ngie]

• Move the bulk majority of the eval function into a check(..) helper function. This improves readability -- and in theory performance -- quite a bit.
• Apply review comment dealing with stderr/stdout files.
• Only cp the scenario file.

[ngie]

Move atf_test_case ${tc_escaped} call out of eval. This seems to give us a 10 second performance gain.

[ngie]

bin/sh/tests/functional_test.sh

64

This is probably part of where the performance degradation is coming from. The bin/date test conversion I have out for review takes <1.5 seconds to complete and this takes 90 seconds to complete, and this is one of the points where it differs from the bin/date test conversion -- along with the find operation done in discover_testcases.

[jilles]

bin/sh/tests/functional_test.sh

64

This command substitution is indeed very slow, and rewriting it to something without forks speeds up a kyua test run to about 30 seconds here (from a 25x to a 5x slowdown).

I notice that testcases in directories containing many testcases are slower. It appears that the list of testcases in the directory is regenerated for every testcase. This should not be needed.

Here is the faster version of the substitution I tried:

escape_tc()
{

local - IFS word notfirst
set -f
IFS=/
set -- $tc@
IFS=_
tc_escaped="$*"
IFS=.
set -- $tc_escaped
tc_escaped= notfirst=
for word do
        tc_escaped=$tc_escaped${notfirst:+__}$word
        notfirst=1
done
IFS=-
set -- $tc_escaped
tc_escaped= notfirst=
for word do
        tc_escaped=$tc_escaped${notfirst:+____}$word
        notfirst=1
done
tc_escaped=${tc_escaped%@}

}

Note that this becomes much simpler if you accept that . and - are changed to a single character instead of multiple.

A simpler alternative you could try (I have not tried this myself):
tc_escaped=$(sed -e 's,/,_,g' -e 's,\.,,g' -e 's,-,__,g' <<EOF
$tc
EOF
)

[ngie]

In D547#29, @jilles wrote:

This command substitution is indeed very slow, and rewriting it to something without forks speeds up a kyua test run to about 30 seconds here (from a 25x to a 5x slowdown).

I notice that testcases in directories containing many testcases are slower. It appears that the list of testcases in the directory is regenerated for every testcase. This should not be needed.

Agreed.

I think part of the reason why things are less optimal at one level is because kyua grabs the list of testcases from libatf-sh.subr, then adds the testcases to the list of available testcases per run in main(..):

759 # Call the test program's hook to register all available test cases.
760 atf_init_test_cases

From a performance perspective it makes a lot more sense (like I did before with this) to generate the testcases at build time with make, then produce a static atf-sh script.

I noticed that there are some suboptimal things that libatf-sh.subr does as well that could be micro-optimized. I'll do some performance tests and post the changes in a pull request after I do some performance testing.

Here is the faster version of the substitution I tried:

escape_tc()
{

local - IFS word notfirst
set -f
IFS=/
set -- $tc@
IFS=_
tc_escaped="$*"
IFS=.
set -- $tc_escaped
tc_escaped= notfirst=
for word do
        tc_escaped=$tc_escaped${notfirst:+__}$word
        notfirst=1
done
IFS=-
set -- $tc_escaped
tc_escaped= notfirst=
for word do
        tc_escaped=$tc_escaped${notfirst:+____}$word
        notfirst=1
done
tc_escaped=${tc_escaped%@}

}

I used this as an inspiration for what I eventually did. I realized a few things:

1. atf_get_srcdir never changes, so I made it a global, thus eliminating a lot of unnecessary variable assignments.
2. When I originally wrote the escaping operation with sed, the testcases were "discovered" from /usr/tests/bin/sh . Now that they're relative to functional areas (e.g. builtins -> /usr/tests/bin/sh/builtins, etc), the "/" no longer matters, and thus applying a "basename(3)"-like operation with the shell variable replacement built-in is the most optimal way to deal with this. Furthermore, there's no reason why the "." needs to be escaped as it's just the exit code, so I applied another shell replacement operation to chop off the "." and the exit code. So that leaves "-" as the only item needing escaping.

Applying these observations, the wall time I obtained according to time(1) for (cd /usr/tests/bin/sh; time kyua test) was ~14 seconds:

390/390 passed (0 failed)
Committed action 288

real 0m16.324s
user 0m2.454s
sys 0m13.955s

Note: if the testcases are renamed so they no longer contain _, but instead contain -, then the escaping logic can go away, the function becomes 5 lines instead of 19, and the runtime will probably decrease as well. The rename change can be made orthogonal to converting bin/sh/tests over to ATF, or I can do it all in one fell swoop if you like.

Note that this becomes much simpler if you accept that . and - are changed to a single character instead of multiple.

True, but then it becomes considerably less flexible for you and for others when developing new tests. That being said, this point also becomes moot if the observation in my "Note" is applied.

A simpler alternative you could try (I have not tried this myself):
tc_escaped=$(sed -e 's,/,_,g' -e 's,\.,,g' -e 's,-,__,g' <<EOF
$tc
EOF
)

The sed operation was way too slow :( (~60 seconds overall).

[ngie]

Apply observations noted in review to reduce kyua test runtime from 90 seconds to 14 seconds.

[jmmv]

Still LGTM.

bin/sh/tests/functional_test.sh

40

nit: Mark err_file and out_file as local.

42

nit: I like the comment, but why the XXX? If this is supposed to be a TODO, I'd indicate how we'd get rid of this eventually.

[ngie]

I really appreciate the in-depth review -- learned some new important things about how ATF works -- thanks :)!

Change committed in r269902.

bin/sh/tests/functional_test.sh

40

Fixed :)!

42

Good point -- I'll mark this TODO :).