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

Tue Nov 15 09:23:33 UTC 2016

Hello guys,

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

Dne 10.11.2016 v 15:52 Cleber Rosa napsal(a):
> 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!
>

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.

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

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.

Regards,
Lukáš

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 502 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20161115/9a3ffcd0/attachment.sig>