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

[Cleber Rosa]

----- Original Message -----
> From: "Lukáš Doktor" <ldoktor at redhat.com>
> To: avocado-devel at redhat.com, "Jan Scotka" <jscotka at redhat.com>, "Cleber Rosa" <crosa at redhat.com>, "Ademar Reis"
> <areis at redhat.com>, "Amador Pahim" <apahim at redhat.com>, "Lucas Meneghel Rodrigues" <lookkas at gmail.com>
> Sent: Tuesday, January 19, 2016 2:19:00 PM
> Subject: RFC: Static analysis vs. import to discover tests
> 
> 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áš
> 

So the whole proposal, in summary is:

* avocado list: uses static based discovery
* avocado list --unsafe: discover tests by loading/running Python code
* avocado run: discover tests by loading/running Python code, and then,
  run them the same way.

I like that, but I'd also suggest that `avocado run` gets a configuration,
item, something like:

[runner]
discover_safely = False

So that, very paranoid users can decide which method to use to find tests
before they're set to be run.

Having tests executed on a separate process is a very weak "protection",
but notice that this may become more important if we choose to implement
and support safer *execution* of tests, such as allowing the change of
effective user id in the process that runs the tests themselves
(think of something like `--run-as foo`).

Also, we have to be careful about "discovering" tests "unsafely" on a local
system that will eventually be run on a remote system (--remote-*/--vm-*).

Thoughts?
CR.