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

Plugin Asset: reset method pointer when parsing a new class. #3665

Merged

Conversation

willianrampazzo
Copy link
Contributor

If there are multiple test classes in the same file, the current method pointer is not reset when parsing the next test class in the file. This may cause a KeyError exception when there is an assignment in a class attribute in the following class.

Signed-off-by: Willian Rampazzo willianr@redhat.com

@willianrampazzo willianrampazzo added this to the #78 (Outbreak) milestone Mar 19, 2020
@willianrampazzo willianrampazzo added this to Review Requested in Avocado Kanban via automation Mar 19, 2020
@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #3665 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3665      +/-   ##
==========================================
- Coverage   69.69%   69.64%   -0.05%     
==========================================
  Files         135      135              
  Lines       17332    17341       +9     
==========================================
- Hits        12079    12078       -1     
- Misses       5253     5263      +10
Impacted Files Coverage Δ
avocado/plugins/assets.py 93.05% <100%> (+0.04%) ⬆️
avocado/utils/ssh.py 62.22% <0%> (-6.08%) ⬇️
avocado/utils/download.py 30.98% <0%> (-4.23%) ⬇️
...plugins/varianter_cit/avocado_varianter_cit/Cit.py 100% <0%> (+0.48%) ⬆️

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 7b624bb...f9b4dc7. Read the comment docs.

@@ -135,6 +135,9 @@ def visit_ClassDef(self, node): # pylint: disable=C0103
if self.klass and node.name != self.klass:
return

# if we are parsing a new class, reset the curret method pointer
if self.current_klass != node.name:
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it make more sense to always reset the current_method when setting current_klass? My reasoning is that the ClassDef will always be visited before the FunctionDef, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works too, with less caution. I have removed the if and force pushed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double-checked just to make sure, and as far as I could test, ClassDef executes before FunctionDef and only once for each class.

If there are multiple test classes in the same file, the current
method pointer is not reset when parsing the next test class in
the file. This may cause a KeyError exception when there is an
assignment in a class attribute in the following class.

Signed-off-by: Willian Rampazzo <willianr@redhat.com>
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.

LGTM, thanks!

@clebergnu clebergnu merged commit 5ef0f3c into avocado-framework:master Mar 24, 2020
@willianrampazzo willianrampazzo deleted the fix_asset_pluging branch March 31, 2020 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Avocado Kanban
  
Review Requested
Development

Successfully merging this pull request may close these issues.

[BUG] In a corner case, when tearDown exists, avocado assets fetch command crashes.
2 participants