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

Thu Nov 10 14:52:28 UTC 2016

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'.

> 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*.

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

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

> 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!

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]

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