t3302-notes-index-expensive.sh and t3419-rebase-patch-id.sh need time in /usr/bin

Hi folks

t3302-notes-index-expensive.sh and t3419-rebase-patch-id.sh need "time " to be in "/usr/bin", however on my system it is in "/bin".

Can't this be checked for?

Bye, Jojo 

This mini series enables the use of the lazy prereq framework for tests which need a (non-builtin) time command.

1/4 is a bugfix 2/4 allows lazy prereq test scripts to set variables for later use 3/4 implements the prereq 4/4 uses it

This should solve the problem reported by Joachim Schmitz.

3/4 could be done in various different ways, see the comments there.

Michael J Gruber (4):
  t3419-rebase-patch-id: heed USR_BIN_TIME prereq
  test-lib: allow variable export from lazy prereq tests
  test-lib: provide lazy TIME_COMMAND prereq
  t3302,t3419: use the TIME_COMMAND prereq

 t/t3302-notes-index-expensive.sh | 5 ++---
 t/t3419-rebase-patch-id.sh       | 7 +++----
 t/test-lib-functions.sh          | 6 ++++--
 t/test-lib.sh                    | 7 +++++++
 4 files changed, 16 insertions(+), 9 deletions(-)

t3419 sets the t3419-rebase-patch-id.sh prereq based on the availability of /usr/bin/time but calls the binary unconditionally (in debug mode).

Make it run the timing only when the prereq is matched.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t3419-rebase-patch-id.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Michael J Gruber <git@drmicha.warpmail.net> writes:

I do not think we should ship our tests with too many test_debug in the first place (it is something you as a developer who are trying to figure out why your change broke tests can insert into tests). It might be useful to be able to say "sh ./t1234-*.sh -d" and see debug output that somebody else thought that it might be useful, so I wouldn't say we should remove all existing test_debug, but at the same time, if a developer finds existing test_debug does not work for him (either what the debug output gives him is insufficient for his needs, or what the debug command uses is not available on his system), he should be willing to update (and capable of doing so) it to suit his needs. Adding prereq in front of test_debug is simply an act of insanity.

For this particular one, I think this should suffice. If the shell does not have "time" built-in, then timeme can be set to even an empty string, but that is left as an exercise to the readers.

 t/t3419-rebase-patch-id.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Currently, lazy prereq tests are run in a subshell which communicates only the exit code to the outer world.

Run it as a subcommand so that variables can be exported to the test environment.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
I don't think this has any adverse side effects, but I'm begging for
another set of eyeballs to have a look. (Test suite passes, of course.)

 t/test-lib-functions.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Michael J Gruber <git@drmicha.warpmail.net> writes:

The lazy prereqs are designed to be used lazily, in any test that he who wrote lazy-prereq did not anticipate. It is run inside a subshell to make it absolutely sure that whatever it does (like use of shell variables, chdir around) will not be able to affect _any_ calling context that is not anticipated by who writes lazy prerequisite.

Please don't do this.

Some test want to use the time command (not the shell builtin) and test for its availability at /usr/bin/time.

Provide a lazy prereq TIME_COMMAND which tests for /usr/bin/time and /bin/time. If any is found, set TEST_COMMAND_PATH to the first match.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Rather than iterating over 2 or more hardcoded paths, one could use
"test -P time" or allow a make variable TIME_COMMAND_PATH whose
executability is checked by the prereq. I really don't know what's best.

 t/test-lib.sh | 7 +++++++
 1 file changed, 7 insertions(+)

test -P time won't work for me: test -P: unary operator expected

I do have another one in /usr/local/bin, maybe that could get added too?

Joachim Schmitz venit, vidit, dixit 16.10.2012 16:13:

Yikes.

If we introduce a make variable TIME_COMMAND_PATH we can even get rid of 2/4 (but have to change Makefile or t/Makefile).

I don't mind too much. /usr/bin/time and /bin/time should be enough.

Some test want to use the time command (not the shell builtin) and test for its availability at /usr/bin/time.

Provide a lazy prereq TIME_COMMAND which tests for $TEST_COMMAND_PATH, which can be set from config.mak. It defaults to /usr/bin/time.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Here's what the make variable version could look like.
1/4 and 4/4 would stay as is, 2/4 could be dropped.

 t/Makefile    | 5 +++--
 t/test-lib.sh | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

Michael J Gruber <git@drmicha.warpmail.net> writes:

An alternative way to suppress the builtin meaning is to quote it, like \time.

Andreas.

Hi Andreas.  I hope you don't mind my nitpickiness, but ...

On 10/16/2012 06:28 PM, Andreas Schwab wrote:

... to be 100% precise, this quoting trick works because 'time' is a shell keyword, rather than a builtin:

    $ type time
    time is a shell keyword

Regards,
  Stefano

Am 16.10.2012 17:07, schrieb Michael J Gruber:

This avoids the builtin:

	command time $that_command

It works for bash, ksh, zsh, and dash (where the latter doesn't have it as builtin).

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

"command time" works but I think that is because it is not a built-in ;-)

Here is what I read in bash(1):

       command [-pVv] command [arg ...]  Run command with args
              suppressing the normal shell function lookup. Only
              builtin commands or commands found in the PATH are
              executed.

Taken together with this from "COMMAND EXECUTION":

       If the command name contains no slashes, the shell attempts
       to locate it.  If there exists a shell function by that name,
       that function is invoked as described above in FUNCTIONS.  If
       the name does not match a function, the shell searches for it
       in the list of shell builtins.  If a match is found, that
       builtin is invoked.

       If the name is neither a shell function nor a builtin, and
       contains no slashes, bash searches each element of the PATH
       for a directory containing an executable file by that name.

I suspect "command printf 'a b c\n'" would not use $HOME/bin/printf even when I have $HOME/bin early in my $PATH (nor /usr/bin/printf for that matter).

Michael J Gruber <git@drmicha.warpmail.net> writes:

As we have t/perf these days, I suspect that we should aim to remove these uses of /usr/bin/time in the main testsuite instead.

The one in 3419 was "run this command, and while at it run it under 'time'" but it is only inside test_debug and I do not think it gives anything more than curiosity values.

The one in 3302 is used to compare two runs (one without and one with notes tree), so it has a little more value than just curiosity, but its value inside the main test suite is still highly dubious. It does not have any "under this value the test passes" criteria.

When the performance of having to look up notes tree really matters, it shouldn't be done inside the main test suite that is designed to be runnable unattended and the only check the humans do is to see their "ok/fail" output.

In short, what you would get out of /usr/bin/time simply is not a good match inside the context of these unit tests.

Use the TIME_COMMAND prereq in both tests so that time from several paths can be used.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t3302-notes-index-expensive.sh | 5 ++---
 t/t3419-rebase-patch-id.sh       | 7 +++----
 2 files changed, 5 insertions(+), 7 deletions(-)