-
Notifications
You must be signed in to change notification settings - Fork 20
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
Test Markers as Test Fields #141
Conversation
628ef91
to
e35cb66
Compare
e35cb66
to
cce8cf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since new functionality is being added it would be great to keep the test coverage on a good shape and have tests updated to consider the new code.
I am sending this as a comment since I am not really involved on the project as I used to be.
Other than the suggestion of dropping an non-related change and adding tests I believe it is good to go.
@@ -423,7 +423,7 @@ def get_requirement_field_values(config, requirement): | |||
|
|||
def update_testcase_fields(config, testcase): | |||
"""Apply testcase fields default values and transformations.""" | |||
if testcase.docstring and not type(testcase.docstring) == str: | |||
if testcase.docstring and not isinstance(testcase.docstring, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason that this is needed? I feel this should not be part of the PR since we are trying to introduce pytest markers support and not change how the docstring is parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elyezer Not needed as part of PR, but the checks are not passing without this change due to flake8 rules! Also there is no harm in doing this change :)
e0448a9
to
6bdf021
Compare
@elyezer Added unit tests to improve the coverage and replied to your comment saying its necessary! |
betelgeuse/parser.py
Outdated
|
||
def parse_markers(all_markers=None): | ||
"""Parse the markers.""" | ||
ignore_list = ['parametrize', 'skipif', 'usefixtures', 'skip_if_not_set'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of these seem to be robottelo
-specific.
Also, since you wish to ignore skipif
marker, I'd say you'd like to also ignore rhel_ver_match
, one etc.
My point here is - perhaps the ignore_list
might be implemented as a CLI option rather than a static, hardcoded list?
It would make it much more flexible and easy to add more over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c8f4378
to
2371099
Compare
2371099
to
e7c4c73
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #141 +/- ##
==========================================
+ Coverage 91.46% 92.12% +0.65%
==========================================
Files 6 6
Lines 879 914 +35
==========================================
+ Hits 804 842 +38
+ Misses 75 72 -3 ☔ View full report in Codecov by Sentry. |
e7c4c73
to
d5e9019
Compare
1c5633a
to
c9dbd8c
Compare
Co-authored-by: Roman Plevka <pecival253@gmail.com>
c9dbd8c
to
c47bee7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK,
thanks for addressing my comments
This PR allows Betelgeuse to :
By:
Helps in: