Added test suite attributes #42
Conversation
|
I guess you should only append the arguments in the |
New attributes do not break old code
|
Hi! I made the changes you proposed for backward compatibility. Is it ok now? |
| def __init__(self, name, assertions=None, elapsed_sec=None, | ||
| timestamp=None, classname=None, status=None, category=None, file=None, line=None, | ||
| log=None, group=None, url=None, stdout=None, stderr=None): | ||
| def __init__(self, name, classname=None, elapsed_sec=None, stdout=None, |
There was a problem hiding this comment.
Here you have also changed the signtature
junit_xml/__init__.py
Outdated
| self.stdout = stdout | ||
| self.stderr = stderr | ||
|
|
||
| self.enable = True |
There was a problem hiding this comment.
It seems that it is better to call it is_enabled
junit_xml/__init__.py
Outdated
| self.skipped_message = None | ||
| self.skipped_output = None | ||
|
|
||
| def disable(self): |
There was a problem hiding this comment.
It is not java, so it is not necessary to add setters/getters. If in the future you'd need to add some logic you can easily do that converting is_enabled to a property. So I would drop that if that's was your concern
junit_xml/__init__.py
Outdated
| if output: | ||
| self.skipped_output = output | ||
|
|
||
| def is_disabled(self): |
There was a problem hiding this comment.
Same here, I would drop that in favour of just using is_enabled flag.
| TestSuite( | ||
| 'test', | ||
| [], | ||
| name='test', |
There was a problem hiding this comment.
As you can see after you changed the constructors signature you broke the tests already. That could be someones code :)
There was a problem hiding this comment.
Here, I just add attributes explicitly in constructor signature. I think I don't break anyone's code :___(
There was a problem hiding this comment.
Off course not. My comment was about this: imagine you have a function and a test:
def foo(a, b, c): ...
def test_foo():
foo('1', '2', [])
If you change the signature of foo to foo(a, c, b) you break the client code and the test as well. And I thought that you realised and fixed that problem just modifying the test:
def test_foo():
foo(a='1', b='2', c=[])
So the test now works but still there are guys out there whose code broke :) So the failing test was a red flag here. If you did not - then it's fine.
| test_case_attributes['name'] = decode(case.name, encoding) | ||
| if case.assertions: | ||
| test_case_attributes['assertions'] = decode(case.assertions, encoding) | ||
| test_case_attributes['assertions'] = "%d" % case.assertions |
There was a problem hiding this comment.
Can you comment that change?
Added explanatory comment about 'assertions' attribute
test_junit_xml.py
Outdated
| def test_attribute_disable(self): | ||
| tc = TestCase('Disabled-Test') | ||
| tc.disable() | ||
| tc.is_enabled=False |
There was a problem hiding this comment.
PEP8 :) Should be spaces around =
test_junit_xml.py
Outdated
| def test_init_disable(self): | ||
| tc = TestCase('Disabled-Test') | ||
| tc.disable() | ||
| tc.is_enabled=False |
There was a problem hiding this comment.
PEP8 :) Should be spaces around =
2 participants
See Junit 10 XSD:
https://github.com/jenkinsci/xunit-plugin/blob/master/src/main/resources/org/jenkinsci/plugins/xunit/types/model/xsd/junit-10.xsd#L92