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

[Cleber Rosa]

----- Original Message -----
> From: "Jan Scotka" <jscotka at redhat.com>
> To: "Lukáš Doktor" <ldoktor at redhat.com>, avocado-devel 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: Wednesday, January 20, 2016 6:31:17 AM
> Subject: Re: RFC: Static analysis vs. import to discover tests
> 
> Hi,
> I think it is perfect solution,
> because exactly as you said, if I want to something to run I want it and
> I accept my fate :-) so static analysis is overhead, dynamic loading is
> more danger, but it is exactly what I want.
> One cons is, when running tests on VM/remote machine, in that case,
> locally, it should not run it, but as you mentioned for list command it
> will be static, and in that case it is good reason to not load the test
> module. because I'll run it on remote machine.
> This dynamic loading also solve that issue with inheritance causing
> infinite loop directly
> https://github.com/avocado-framework/avocado/issues/961

Jan,

I also like the proposal, but keep in mind that the infinite loop, while
triggered by running some tests as SIMPLE tests, instead of INSTRUMENTED
tests, actually lives in avocado.main(). There's been some discussion on
that at

https://github.com/avocado-framework/avocado/pull/968

Cheers!
CR.

>      Thanks&Regards
>      Honza
> 
> 
> 
> On 01/19/2016 05:19 PM, Lukáš Doktor wrote:
> > 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áš
> 
> 
> --
> Jan Scotka <jscotka at redhat.com>  RHCE
> QA daemons Team
> irc (jscotka at): #brno, #urt, #qa, #errata, #devel
> 
>