[Avocado-devel] RFC: Static analysis vs. import to discover tests

[Lukáš Doktor]

Hello guys,

as the topic suggests, we'll be talking about ways to discover whether 
given file is an Avocado test, unittest, or simple test.


History
=======

At the beginning, Avocado used `inspect` to get all module-level classes 
and queried if they were inherited from avocado.Test class. This worked 
brilliantly, BUT it requires the module, to be actually loaded, which 
means the module-level code is executed. This is fine when you intend to 
run the test anyway, but it's potentially dangerous, when you just list 
the tests. The problem is, that simple "avocado list /" could turn into 
'os.system("rm -rf / --no-preserve-root")' and we clearly don't want that.

So couple of months back, we started using AST and static analysis to 
discover tests.

The problem is, that even the best static analysis can't cover 
everything our users can come up with (for example creating Test classes 
dynamically during import). So we added a way to override this behavior, 
docstring "avocado: enable" and "avocado: disable". It can't handle 
dynamically created `test` methods, but it can help you with inheritance 
problems.

Anyway it's not really friendly and you can't execute inherited `test` 
methods, only the ones defined in your class. Recently we had questions 
from multiple sources asking "Why is my class not recognized?" Or "Why 
is my test executed as SIMPLE test?".


My solution
===========

I don't like static analysis. It's always tricky, it can never achieve 
100% certainty. So I'm proposing moving back to loading the code, while 
keeping the static analysis for listing purposes. So how would that work?


avocado list
------------

would use static analysis to discover tests. The output would be:

```
SIMPLE         /bin/true
INSTRUMENTED   examples/tests/gdbtest.py:GdbTest.test_start_exit
INSTRUMENTED   examples/tests/gdbtest.py:GdbTest.test_existing_commands_raw
INSTRUMENTED   examples/tests/gdbtest.py:GdbTest.test_existing_commands
INSTRUMENTED 
examples/tests/gdbtest.py:GdbTest.test_load_set_breakpoint_run_exit_raw
?INSTRUMENTED? multiple_inheritance.py:Inherited.*
?INSTRUMENTED? multiple_inheritance.py:BaseClass.*
?INSTRUMENTED? library.py:*
?INSTRUMENTED? simple_script.py:*
```

Where ?xxx? would be yellow and it stands for "It looks like 
instrumented test, but I can't tell".

When the user makes sure, he wants to run those tests and they are not 
nasty, he would use `avocado list --unsafe`, which would actually load 
the modules and give precise results:

```
SIMPLE       /bin/true
INSTRUMENTED examples/tests/gdbtest.py:GdbTest.test_start_exit
INSTRUMENTED examples/tests/gdbtest.py:GdbTest.test_existing_commands_raw
INSTRUMENTED examples/tests/gdbtest.py:GdbTest.test_existing_commands
INSTRUMENTED 
examples/tests/gdbtest.py:GdbTest.test_load_set_breakpoint_run_exit_raw
INSTRUMENTED multiple_inheritance.py:Inherited.test1
INSTRUMENTED multiple_inheritance.py:Inherited.test2
INSTRUMENTED multiple_inheritance.py:Inherited.test3
SIMPLE       simple_script.py
```

You can see that this removed the `library.py`, it also discovered that 
the `multiple_inheritance.py:BaseClass`` is not test. On the other hand, 
it discovered that `multiple_inheritance.py:Inherited` holds 3 tests. 
This result is, what `avocado run` would actually execute.


avocado run
-----------

I don't think `avocado run` should support safe and unsafe option, not 
even when running `--dry-run`. When one decides to run the tests, he 
wants to run them, so I think `avocado run` should always use the unsafe 
way using `inspect`. That way there are no surprises and complex 
workarounds to get things done.


Summary
=======

The current way is safe, `avocado list` and `avocado run` are always in 
sync, but it's strange to users as they write tests, they use multiple 
inheritance and they need BaseClasses. We have a solution, which 
requires docstrings, but it's just not flexible enough and can never be 
as flexible as actual loading.

The proposed solution allows using `avocado list` safely, while `avocado 
run` would execute what test developer wrote, without trying to be too 
smart. The only cons I see is, that in some cases the `avocado list` 
might fail to list all tests, but it should always notify about the 
possibility.

I hope it's understandable, feel free to add your comments and other 
possible solutions,

Lukáš