Conversation
FlTr
force-pushed
the
enable-inheritance
branch
from
16553e8 to
d2e7e91
Compare
EnricoMi
left a comment
EnricoMi
left a comment
There was a problem hiding this comment.
Some comments on approach design.
junitparser/junitparser.py
Outdated
| __test__ = False | ||
|
|
||
| testcase = TestCase | ||
| root = None |
There was a problem hiding this comment.
Why not
| root = None | |
| root = JUnitXml |
There was a problem hiding this comment.
In theory that would be also my preferred way, but JUnitXml class is defined later in the same file and therefore can't be used here to initialize static member.
There was a problem hiding this comment.
I think we then can turn this static member into a class member, code should work either way
| root = None |
junitparser/junitparser.py
Outdated
| cls = JUnitXml | ||
| if TestSuite.root is not None and issubclass(TestSuite.root, JUnitXml): | ||
| cls = TestSuite.root | ||
| result = cls() |
There was a problem hiding this comment.
This then simplifies to
| cls = JUnitXml | |
| if TestSuite.root is not None and issubclass(TestSuite.root, JUnitXml): | |
| cls = TestSuite.root | |
| result = cls() | |
| result = TestSuite.root() |
There was a problem hiding this comment.
To align with other places, TestSuite.root should be accessed as self.root.
|
|
||
| def __init__(self, name=None): | ||
| super().__init__(name) | ||
| TestSuite.root = JUnitXml | ||
|
|
There was a problem hiding this comment.
Could be done similar to testcase, or is root different?
| def __init__(self, name=None): | |
| super().__init__(name) | |
| TestSuite.root = JUnitXml | |
| root = JUnitXml | |
| xml = parseString(text) # nosec | ||
| content = xml.toprettyxml(encoding="utf-8") | ||
| if isinstance(file_or_filename, str): | ||
| with open(file_or_filename, encoding="utf-8", mode="wb") as xmlfile: |
There was a problem hiding this comment.
This is unrelated, please revert.
There was a problem hiding this comment.
Agree that this is unrelated. Will revert if requested, but in my case it raises a ValueError "binary mode doesn't take an encoding argument". Suggestion?
There was a problem hiding this comment.
You are right, given content is bytes and mode="wb", the encoding="utf-8" is not needed.
Strange we do not see this in tests. Is that specific to some Python version?
Do you mind opening a new pull request where tests fail on this issue and then fix it as above?
There was a problem hiding this comment.
Can't tell you. But I can see it locally but not in my project CI instance.
Will revert here and try to manage a separate PR.
FlTr
force-pushed
the
enable-inheritance
branch
from
e61b4f8 to
dc84d2e
Compare
FlTr
force-pushed
the
enable-inheritance
branch
from
dc84d2e to
4a26fa3
Compare
EnricoMi
left a comment
EnricoMi
left a comment
There was a problem hiding this comment.
Looks good!
Can you add some tests that would fail without this change (when adding or iterating over testsuites)?
|
Added tests to verify iteration and add operations return the correct, potentially inherited, TestSuites / JUnitXml instances. |
EnricoMi
left a comment
EnricoMi
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the contribution!
|
@weiwei any objections? |
3 participants
Follow up on #143, now it should be possible to properly derive all 3 classes (JUnitXml, TestSuite, TestCase) without loosing information while e.g. adding 2 testsuites to each other.