On Thu, Jun 1, 2017 at 10:32 PM, Lucas Meneghel Rodrigues <[email protected]> wrote: > My 2 cents, I agree with Cleber here 100%. The current docstring tags are > more of a workaround, and ideally none of that would be necessary, and it's > worth to spend more time trying to get rid of it. > > About using only AST static analysis, this is the one reason why the > docstring tags are being used, it this approach is not used then they are > not necessary to begin with. I believe the current approach can scale up to > make the tags obsolete, we can see if astroid [1], the base pylint library > could help us reach this goal. > > [1] https://github.com/PyCQA/astroid > > > On Thu, Jun 1, 2017 at 7:22 AM Lukáš Doktor <[email protected]> wrote: >> >> Dne 30.5.2017 v 11:02 Amador Pahim napsal(a): >> > Hello, >> > >> > This came up as an issue and I believe it deserves some discussion as >> > there are some different approaches to implement the feature. >> > >> > Motivation >> > ========== >> > >> > Currently we can discover test classes that does not inherit directly >> > from `avocado.Test`. To do so, we rely on the inclusion of a docstring >> > (`:avocado: enable`) in the mentioned class. Example below. >> > >> > File `/usr/share/avocado/tests/test_base_class.py`:: >> > >> > from avocado import Test >> > >> > >> > class BaseClass(Test): >> > >> > def test_basic(self): >> > pass >> > >> > File `/usr/share/avocado/tests/test_first_child.py`:: >> > >> > from test_base_class import BaseClass >> > >> > >> > class FirstChild(BaseClass): >> > """ >> > :avocado: enable >> > """ >> > >> > def test_first_child(self): >> > pass >> > >> > In the example above, if we ask Avocado to list the tests from >> > `test_first_child.py`, `FirstChild.test_first_child` will be listed and >> > the `BaseClass.test_basic` won't:: >> > >> > $ avocado list test_first_child.py >> > INSTRUMENTED test_first_child.py:FirstChild.test_first_child >> > >> > The request is that, in such cases, we have a way to include the >> > `BaseClass.test_basic` into the results. >> > >> > Proposal >> > ======== >> > >> > To include the parent classes into the discovery results, we have three >> > main aspects to consider: >> > >> > - How to flag that we want that behaviour? >> > The proposal is the creation of a new docstring `recursive`. >> > Example:: >> > >> > class FirstChild(BaseClass): >> > """ >> > :avocado: recursive >> > """ >> > ... >> I do like this directive. >> >> > >> > Alternative options here are: 1)command line option; 2)make the >> > recursive discovery the default behavior. >> Given the history and complexity of such behavior, I'd only allow this >> when the docstring directive does that. We can think about command-line >> option to change it as well, but I don't think we should change the >> default. We are not running the code, therefor our discovery is >> different. People should cope with that.
Ok. se we agree in using the docstring ':avocado: recursive' to trigger the recursion. >> >> > >> > - How deep is the recursion? >> > The proposal is that the recursion goes all the way up to the class >> > inheriting from `avocado.Test`. >> > Alternative option here is to discover only the first parent of the >> > class flagged with `recursive`. If the parent class also has the same >> > docstring, then we go one more level up, and so on. >> I don't think we should limit this as dynamic loaders (unittest) always >> go all the way down. Later, though, we could come up with other >> docstring directive to break the chain if needed, but I'd not give it a >> priority (as people can simply inherit from different classes to get the >> same functionality). Ok, given the feedbacks, seems there's a big preference for the complete recursion (instead of one-level-up recursion). >> >> > >> > - Will the recursion respect the parents docstrings? >> > The proposal is that we do respect the docstrings in the parents when >> > recursively discovering. Example: >> > >> > File `/usr/share/avocado/tests/test_base_class.py`:: >> > >> > from avocado import Test >> > >> > >> > class BaseClass(Test): >> > >> > def test_basic(self): >> > pass >> > >> > File `/usr/share/avocado/tests/test_first_child.py`:: >> > >> > from test_base_class import BaseClass >> > >> > >> > class FirstChild(BaseClass): >> > """ >> > :avocado: recursive >> > """ >> > >> > def test_first_child(self): >> > pass >> > >> > Will result in:: >> > >> > $ avocado list test_first_child.py >> > INSTRUMENTED test_first_child.py:FirstChild.test_first_child >> > INSTRUMENTED test_first_child.py:BaseClass.test_basic >> > >> > While: >> > >> > File `/usr/share/avocado/tests/test_base_class.py`:: >> > >> > from avocado import Test >> > >> > >> > class BaseClass(Test): >> > """ >> > :avocado: disable >> > """ >> > >> > def test_basic(self): >> > pass >> > >> > File `/usr/share/avocado/tests/test_first_child.py`:: >> > >> > from test_base_class import BaseClass >> > >> > >> > class FirstChild(BaseClass): >> > """ >> > :avocado: recursive >> > """ >> > >> > def test_first_child(self): >> > pass >> > >> > Will result in:: >> > >> > $ avocado list test_first_child.py >> > INSTRUMENTED test_first_child.py:FirstChild.test_first_child >> > >> My view on this is it should not. If we want this behavior I think we >> should come up with a different tag to break the chain, but as mentioned >> earlier I don't see it as a priority. >> >> Anyway an example to show why I think we should not stop discovering on >> "disable" Ok, so the agreement is that we don't break the recursion on 'disabe' docstring. At this point, I don't think we need a new docstring to break the chain. If someone ask for that in the future, we can re-visit this topic. >> >> ```python >> class BaseNetworkTests(Test): >> """ >> :avocado: disable >> """ >> def test_foo(self): >> pass >> >> def test_bar(self): >> pass >> >> class MyNetworkTests(BaseNetworkTests): >> """ >> :avocado: recursive >> """ >> ``` >> >> When in one file, running `avocado $that_file` executes only >> MyNetworkTests combining tests from MyNetworkTests and BaseNetworkTests, >> but the BaseNetworkTests should not be executed (as it's disabled). Ok >> >> > The alternative option is that the discovery ignores the parents >> > docstrings when discovering recursively, meaning that the >> > `:avocado: disable` (or any other current or future available >> > docstrings) would have no effect in the recursive discovery. >> > >> > Expected Results >> > ================ >> > >> > The expected results of this RFC is to have a well defined behavior for >> > the recursive discovery feature. >> > >> > The expected result of the feature itself is to provide users more >> > flexibility when creating the Avocado tests and consequent Avocado >> > command lines. >> So my expected result would by: >> >> By default - our static analysis which does not descend to parents >> On :avocado: recursive - act as dynamic loader (descend all parents of >> all parents till the end of the graph without any exceptions and create >> a set of `test*` classes) >> >> eventually if asked by users, I'd support optional argument to switch >> between those two modes by default (still the docstring takes >> precedence) and if people really wants I'd support another tag to break >> the chain (:avocado: stop-recursion or so). >> >> Anyway I'm open to other suggestions, this is only my basic view, >> Lukáš >> >> > >> > Additional Information >> > ====================== >> > >> > Avocado uses only static analysis to examine the files and this feature >> > should stick to this principle in its implementation. >> > >> > >> > >> > >> > Looking forward to read your comments. >> > -- >> > apahim >> > >> >> >
