Re: [Avocado-devel] [RFC] white-spaces in test references

[Lukáš Doktor]

Hello guys,




Dne 10.11.2016 v 15:52 Cleber Rosa napsal(a):

basically Cleber covered it all, let me just add some remarks and summarize in the end.

On 11/10/2016 10:31 AM, Amador Pahim wrote:

Hi folks,

Please consider this series of tests. Comments are after them.

-------------------------------------------------------------------------------
INSTRUMENTED:
$ cp examples/tests/passtest.py examples/tests/pass\ test.py

Shell does the escaping, avocado gets 'examples/tests/pass test.py'.

Example 1:

$ avocado run 'pass test.py'
PASS

Single quotes tells shell to not mess with it, avocado gets 'pass test.py'.

Example 2:

$ avocado run 'examples/tests/pass test.py'
PASS

Single quotes again, avocado gets 'examples/tests/pass test.py'

Example 3:

$ avocado run pass\ test.py
PASS

Shell does the escaping, avocado gets 'pass test.py' (*exactly* like
example #1).

Example 4:

$ avocado run examples/tests/pass\ test.py
PASS

Shell escapes whitespace, avocado gets 'examples/tests/pass test.py',
(*exactly* like example #2).

Example 5:

$ avocado run 'pass\ test.py'
Unable to resolve reference(s) 'pass\ test.py' with plugins(s) 'file',
'external', try running 'avocado list -V pass\ test.py' to see the
details.

Single quotes tells the shell to not mess with it, avocado get 'pass\
test.py'.

So far so good.

Example 6:

$ avocado run 'examples/tests/pass\ test.py'
PASS

Single quotes tells the shell to not mess with it, avocado get
'examples/tests/pass\ test.py' and still finds a test.  This is not
expected, it's clearly a *BUG*.

This was introduced in order to allow supplying arguments. More on this in the simple-tests-with-args. The only bug I see here is that it should only allow this for simple tests while it allows it here for all file-based test types, which is a bug.

-------------------------------------------------------------------------------

SIMPLE TESTS
$ cd /tmp/

It is not clear what is the real file name you created for this example.
 I'm assuming it's '/tmp/test script.sh'.

Example 7:

$ avocado run 'test script.sh'
PASS

Avocado gets 'test script.sh', finds it at the current working directory
and runs it.  We have supported looking for tests (simple or
instrumented) at the current working directory besides the configured
test dir, so the non-absolute path is OK.

Example 8:

$ avocado run '/tmp/test script.sh'
ERROR
 Running '/tmp/'/tmp/test script.sh''
 ...
 File "/home/apahim/src/avocado/avocado/utils/process.py", line 383,
in _init_subprocess
      raise exc
 OSError: File '/tmp/'/tmp/test' not found

Avocado gets '/tmp/test script.sh', should have found it, should have
run it.  Clearly a bug.

Yes, this is the bug the card you're working on is about. I sent you my notes and it's due to `@filename` test property which uses escaped name. I fixed it (quick&dirty way) in my tree and it worked well.

Example 9:

$ avocado run test\ script.sh
PASS

Avocado gets 'test script.sh'.  No surprises here.

Example 10:

$ avocado run /tmp/test\ script.sh
ERROR
 Running '/tmp/'/tmp/test script.sh''
 ...
 File "/home/apahim/src/avocado/avocado/utils/process.py", line 383,
in _init_subprocess
      raise exc
 OSError: File '/tmp/'/tmp/test' not found

Avocado gets '/tmp/test script.sh'.  Should have found it and run it.
Clearly the same bug as in example #8.

Example 11:

$ avocado run 'test\ script.sh'
PASS

Avocado gets 'test\ script.sh', which doesn't exist.  The additional
escaping Avocado attempts to do, is, IMO, a bug.

Again, it's for the arguments discovery...

Example 12:

$ avocado run '/tmp/test\ script.sh'
PASS

Avocado gets '/tmp/test\ script.sh', which doesn't exist.  Same bug as
in example #11.

SIMPLE TESTS WITH ARGUMENTS:

Example 13:

$ avocado run 'test script.sh arg1'
Unable to resolve reference(s) 'test script.sh arg1' with plugins(s)
'file', 'external', try running 'avocado list -V test script.sh arg1'
to see the details.

Besides the other implications, this is super confusing.  Does the user
wants to run:

1) "test", with arguments "script.sh" and "arg1"
2) "test script.sh", with arguments "arg1"
3) "test script.sh arg1"

Deciding that based on the presence of either "test" or "test script.sh"
or "test script.sh arg1" is possible, bug still confusing and error prone.

Example 14:

$ avocado run '/tmp/test script.sh arg1'
Unable to resolve reference(s) '/tmp/test script.sh arg1' with
plugins(s) 'file', 'external', try running 'avocado list -V /tmp/test
script.sh arg1' to see the details.

Without the support for passing arguments, this would be crystal clear.
Avocado gets '/tmp/test script.sh arg1', which doesn't exist, so the
message *would be* correct.

With the current support for passing arguments, this is clearly a bug.

Example 15:

$ avocado run 'test\ script.sh arg1'
PASS, script receives arg1.

Example 16:

$ avocado run '/tmp/test\ script.sh arg1'
PASS, script receives arg1.

This non-standard quoting for *some* cases is broken.  No other words
about it.

-------------------------------------------------------------------------------

Example 8 and Example 10 are affected by an issue in
SimpleTest.filename. This issue is caused by the pipes.quote(filename)
call in the FileLoader. The pipes.quote(filename) is putting single
quotes around the entire filename and making
os.path.abspath(filename), which is present in SimpleTest.filename, to
return the incorrect location. Btw, the same issue is affecting
filenames with non-ascii characters.

Right.

In order to fix this issue, we have some options, like 'handle the
quoted filename coming from the loader inside the
SimpleTest.filename', which fixes Examples 8 and 10 and does not
change anything else, or 'to remove the pipes.quote(filename) from the
loader' which makes the syntax on Examples 7, 8, 9 and 10 invalid (so
all white-spaces in filenames have to be escaped AND the test
reference have to be enclosed inside quotes when the filename contains
white-spaces, like in examples 11, 12, 15 and 16).

Doing the escaping inside Avocado is counter intuitive.  We should not
attempt to be a shell.  The following should *never* be an issue:

 $ ./foo/bar\ baz.sh
 SUCESS

 $ avocado run ./foo/bar\ baz.sh
 ...
 (1/1) './foo/bar baz.sh': PASS (0.01 s)
 ...

But this is an issue:

 $ './foo/bar baz.sh'
 SUCCESS

 $ './foo/bar\ baz.sh'
 bash: ./foo/bar\ baz: No such file or directory

 $ avocado run './foo/bar\ baz.sh'
 ...
 (1/1) ./foo/bar\ baz.sh: PASS (0.01 s)
 ...

Are we in sync up to this point?  This deserves a card in Trello
describing the bug.  The resolution should include tests, such as
running avocado under a shell (as most functional tests do) and passing
test references that exercise the intended behavior.

But this issue raised a new discussion: right now, for both
INSTRUMENTED and SIMPLE tests, we accept non-escaped white-spaces in
the test reference, as long as the test reference is enclosed into
quotes (Examples 1, 2, 7 and 8). But there is one exception: if we

Let's change the wording a bit: the escaping has been done by the shell
(enclosed in quotes).  Avocado gets a test reference that contains white
spaces (which should be fine).

have a SIMPLE test with white-spaces in the filename AND arguments,
then the white-spaces in the filename have to be escaped (Examples 15
and 16). This change of behaviour based on the presence/absence of
arguments seems confusing from the user perspective. This syntax

Agreed.  The presence of a feature (passing arguments to simple tests)
should not change the overall expectation/behavior for the application.

(Examples 1, 2, 7 and 8) maybe can make sense for INSTRUMENTED tests,
since we don't have arguments there, but it does not make sense for
SIMPLE tests, because we do support arguments in the test references
for SIMPLE tests.

So, before sending a new Pull Request, I'd like to have some feedback
about this. What are the valid syntaxes that we have to support and
what syntaxes should not be valid from the list of examples above?
Should we keep all of them as they are currently and just fix Examples
8 and 10? Or the Examples 7, 8, 9 and 10 are looking wrong for you as
well?

Unless we can deliver a way to support "simple tests + arguments" without:

1) Confusing change of expectation and behavior for references on other
test types
2) Avocado acting as a shell (doing quoting)

I recommend we drop support for passing arguments to simple tests.

If a case can be made for the feature (support for simple tests +
arguments) *and* at the same time removing the two points listed
previously, we can consider keeping the feature.

Best,
--
apahim

Thanks for the thorough analysis of this issue!

Imagine I want to keep my CI in one job, which means to run:

1. unattended_install
2. boot
3. qemu-iotest 001
4. qemu-iotest 123
5. run-kvm-unit-test 001
6. run-kvm-unit-test 123











Regards,
Lukáš

As I said before, the simple tests + args is a neat feature. I used to use it a lot before I split my checks into multiple jobs. Now I don't combine different simple tests, but I rather run multiple external-runner jobs. Still I believe it is a useful feature and some people might actually depend on it. If we had the support to specify test type, it would be fine as we could specifically say: `avocado run VT://unattended_install VT://boot SIMPLE://qemu iotest\\ 001 ...` (and say that simple test can accept params). Alternatively we could define other separator for extra params to the test, let's say `avocado run qemu-iotest:001` would be converted to `qemu-iotest` test with argument `001`, unless we use double `::`. This would make things less magical and more understandable. Anyway it is possible to run those tests even now, if we make sure we don't have clashes by using custom external runner which simply does `exec $*`. Then we can use `avocado run --loaders file external:run_simple_with_args.sh -- unattended_install boot "qemu-iotest 001" "qemu-iotest 002" ...` (not it'd require the extra quotation, but it's fine as we're using our own script) So to wrap it up, I'm fine with removing the support, it'll be faster and less confusing, but I'd welcome to describe the ways to run simple tests with args as it is pretty useful. (originally I preferred that to the external runner, but nowadays I'm used to external runner so we can abuse it in order to support simple tests with args). One last note regarding external runner vs. simple tests, IIRC the external runner does not support the extra bash API we have for simple tests (logging, test status WARN, ...). I think if we get rid of simple tests with args we should allow this API in external runner tests as well.

signature.asc
Description: OpenPGP digital signature