Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avocado.skip* usable on classes [v2] #3570

Merged
merged 1 commit into from Feb 19, 2020

Conversation

richtja
Copy link
Contributor

@richtja richtja commented Feb 18, 2020

This commit expand the avocado.skip* decorators from methods to classes.
When a user uses skip decorator on the class, the decorator is applied
to every class method.

Reference: #3473
Signed-off-by: Jan Richter jarichte@redhat.com


Changes from v1 (#3566):

  • Deletion of unused variable value
  • Usage of isinstance(obj, type)

This commit expand the avocado.skip* decorators from methods to classes.
When a user uses skip decorator on the class, the decorator is applied
to every class method.

Reference: avocado-framework#3473
Signed-off-by: Jan Richter <jarichte@redhat.com>
@richtja richtja changed the title Avocado.skip* usable on classes Avocado.skip* usable on classes [v2] Feb 18, 2020
@richtja richtja added this to Review Requested in Avocado Kanban via automation Feb 18, 2020
@richtja richtja linked an issue Feb 18, 2020 that may be closed by this pull request
@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/avocado-framework-avocado-3570
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@codecov
Copy link

codecov bot commented Feb 18, 2020

Codecov Report

Merging #3570 into master will decrease coverage by 0.02%.
The diff coverage is 58.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3570      +/-   ##
==========================================
- Coverage   70.07%   70.05%   -0.03%     
==========================================
  Files         137      137              
  Lines       16861    16871      +10     
==========================================
+ Hits        11816    11819       +3     
- Misses       5045     5052       +7
Impacted Files Coverage Δ
avocado/core/decorators.py 62.5% <58.82%> (-4.9%) ⬇️
...plugins/varianter_cit/avocado_varianter_cit/Cit.py 99.51% <0%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f0e845...be15d15. Read the comment docs.

Copy link
Contributor

@willianrampazzo willianrampazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code tested, working as expected, LGTM!

@apahim
Copy link
Contributor

apahim commented Feb 18, 2020

While I like the functionality, I think there's room for discussion regarding the concepts here.
When implementing the skip decorators, the consensus was that a test method that is skipped should have no code executed at all. For a test method, no code executed means no setUp(), no testMethod() and no tearDown().
To extend the skip concept to the test classes, I'd expect the test class also not to have any code executed at all. In that case, no __init__(), no log_dir/data_dir initialization, nothing.
To achieve that, looping all the methods of the class and marking them as skip would not be enough. This decorator should, instead, avoid the class of being instantiated, so it would probably act in the loader, not in the test.

Copy link
Contributor

@apahim apahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment :)

@willianrampazzo
Copy link
Contributor

To achieve that, looping all the methods of the class and marking them as skip would not be enough. This decorator should, instead, avoid the class of being instantiated, so it would probably act in the loader, not in the test.

I agree that the implementation you described would fulfill the requirements you described and would be more efficient.

@richtja
Copy link
Contributor Author

richtja commented Feb 18, 2020

@apahim, yes, you are right that skipping class should skip execution of all class code. But I think that to do that, we have to use a completely different approach than with skipping methods. And I am not sure if it's possible to accomplish that with python decorators. We were discussing that, on a meeting today, and it came up an idea that we can use this solution for now, because it handles most of the requirements, and also start looking for a more sophisticated solution that will be skipping whole class code. What do you think?

Copy link
Contributor

@apahim apahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't block this PR, sure, just wanted to make sure we know all the implications here.

@richtja
Copy link
Contributor Author

richtja commented Feb 19, 2020

I created issue #3579 that focus on skipping the whole classes.

Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we can evolve this implementation in a later stage. Thanks for the code and the review guys!

@clebergnu clebergnu merged commit be15d15 into avocado-framework:master Feb 19, 2020
@richtja richtja deleted the skip_v2 branch February 24, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Avocado Kanban
  
Review Requested
Development

Successfully merging this pull request may close these issues.

Make avocado.skip* usable on classes
4 participants