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
| @@ -308,6 +336,7 @@ def __init__(self, name, assertions=None, elapsed_sec=None, | |||
| 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
| @@ -317,6 +346,10 @@ def __init__(self, name, assertions=None, elapsed_sec=None, | |||
| 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
| @@ -342,6 +375,10 @@ def add_skipped_info(self, message=None, output=None): | |||
| 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.
| @@ -73,8 +73,8 @@ def test_single_suite_no_test_cases(self): | |||
|
|
|||
| (ts, tcs) = serialize_and_read( | |||
| 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.
junit_xml/__init__.py
Outdated
| # test cases | ||
| for case in self.test_cases: | ||
| test_case_attributes = dict() | ||
| 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
| @@ -213,7 +213,7 @@ def test_attribute_time(self): | |||
|
|
|||
| 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
| @@ -338,7 +338,7 @@ def test_init_stdout_stderr(self): | |||
|
|
|||
| 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 =
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