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 ]
signature.asc
Description: OpenPGP digital signature
