-
Notifications
You must be signed in to change notification settings - Fork 139
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
Make launchtest junit XML match pytest XML more closely #228
Conversation
The test failure is in test_flake8, but it seems the checker has barfed up an error instead of telling me what I did wrong:
|
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.
@pbaughman I see that APEX still shows up in the API. There's an ongoing effort to homogenize the naming (see #208), so consider re-targeting this PR to that structure and style instead (see #215).
@@ -229,6 +229,12 @@ def run(self): | |||
try: | |||
worker = _RunnerWorker(run, self._launch_file_arguments, self._debug) | |||
results[run] = worker.run() | |||
except unittest.case.SkipTest as skip_exception: | |||
# If a 'skip' decorator was placed on the generate_launch_description function, |
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.
@pbaughman leftover
# If a 'skip' decorator was placed on the generate_launch_description function, | |
# If a 'skip' decorator was placed on the generate_test_description function, |
?
375dcfc
to
d407814
Compare
@hidmic Ok, this PR is now rebased onto The only merge conflicts I hit were in test_xml_output.py - everything else went in cleanly. The CI environment doesn't appear to like my use of 'mock' in test_runner_results.py:
That wasn't added in this PR though - It looks like it's also breaking in #215 - should we re-do this test, or add 'mock' to CI, or something different? |
@pbaughman you mean |
@pbaughman mind to rebase? |
- Use the `classname` attribute in the `testcase` element instead of jamming everything into `name` - Use the `message` attribute for `skipped` elements - Handle the case where @unittest.skip("Skip Message") is added to the generate_test_description function Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
d407814
to
c153c37
Compare
@hidmic Done |
Please double check how the changes are rendered in the Jenkins test results report. |
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.
|
||
results = runner.run() | ||
# Should just get one result, even though there were multiple tests | ||
assert len(results.values()) == 1 |
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.
@pbaughman it'd be nice to check the XML structure too.
class __skipped_test_case: | ||
|
||
def __init__(self): | ||
self._testMethodName = 'skipped_launch' |
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.
@pbaughman meta: doesn't this make the whole XML output rather obscure? It removes all references to the actual tests being skipped.
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.
I don't know about 'rather obscure.' The name of the test file is still in the XML output. Your point is received, though. . .
Upon looking at this a second time, the test run is available at the point we create the SkipResult. We could pass it into the SkipResult
constructor and generate a full XML report that lists each test case as skipped. That's what pytest does if you put a skip decorator on the test class.
# <testsuite name="run1" . . . > | ||
# <testcase classname="TestHost" name="test_0" . . . /> | ||
# </testsuite> | ||
# <testsuites> |
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.
@pbaughman nit: consider putting explanation on a test docstring.
# <skipped message="My reason is foo" . . . /> | ||
# </testcase> | ||
# </testsuite> | ||
# <testsuites> |
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.
@pbaughman nit: consider putting explanation on a test docstring.
# </failure> | ||
# </testcase> | ||
# </testsuite> | ||
# <testsuites> |
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.
@pbaughman nit: consider putting explanation on a test docstring.
# </failure> | ||
# </testcase> | ||
# </testsuite> | ||
# <testsuites> |
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.
@pbaughman nit: consider putting explanation on a test docstring.
Hmm, Jenkins is not picking up any jUnit file when it should. I'll check locally if the tool is even generating them. |
@hidmic I imagine it would have to be a Jenkins run of some package that uses The test |
- When we skip all tests by putting @unittest.skip on the generate_test_description function, we generate the junit XML as though all the tests individually had the @unittest.skip decorator - Improve documentation on new tests Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
@hidmic I think I've addressed all of your PR comments |
Hmm tests do generate jUnit XML but Jenkins is not consuming it. I had not realized before. We need to follow up on this. |
@hidmic Ok - we can hold this PR open until we've identified the problem and then try to include the fix here if it turns out to be an issue with the XML format. |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Alright, it wasn't that XML report were not being picked up but rather that these were not being labeled properly. @pbaughman I'm pull requesting against your branch, pbaughman#1. Mind to review/merge so we get this boy going? |
@pbaughman I took the liberty to push to your branch to get things going, hope that's not an issue for you. |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Ready to be merged after a final re-approval. |
Can you link to a launch test result on Jenkins as an example? |
Of course! Here you can see |
@@ -81,13 +82,12 @@ def all_cases(self): | |||
yield from _iterate_tests_in_test_suite(self.post_shutdown_tests) | |||
|
|||
def __str__(self): |
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.
TODO Pete: reconcile this with this
@@ -83,7 +83,8 @@ def unittestCaseToXml(test_result, test_case): | |||
class needs to be a launch_testing.TestResult class | |||
""" | |||
case_xml = ET.Element('testcase') | |||
case_xml.set('classname', type(test_case).__name__) | |||
full_classname = type(test_case).__module__ + '.' + type(test_case).__name__ |
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.
@hidmic Sorry, I know this question is late to the party, but this will always be launch_testing, right?
I used the module name of the test description function https://github.com/ApexAI/apex_rostest/blob/c8cd0ac6dc5383334fdbfd67e7bfeee62bf37c1f/apex_launchtest/apex_launchtest/loader.py#L97
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.
Follow-up I see in a later commit that you did basically the same thing by generating the test name from the name of the test file. That's what I get for reviewing one commit at a 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.
Sorry, I know this question is late to the party, but this will always be launch_testing, right?
It'll be the name of the package the test is in, which may be provided by a caller or be the base name of the test file.
@@ -98,7 +98,7 @@ def main(): | |||
parser.error("Test file '{}' does not exist".format(args.test_file)) | |||
|
|||
args.test_file = os.path.abspath(args.test_file) | |||
test_module = _load_python_file_as_module(args.test_file) | |||
test_module = _load_python_file_as_module(args.test_name, args.test_file) |
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.
I guess this works too - usually the only time we care about the test name is when we run in CI and there we have launch_testing_cmake to give us a good test name.
For reference, https://github.com/ApexAI/apex_rostest/blob/c8cd0ac6dc5383334fdbfd67e7bfeee62bf37c1f/apex_launchtest/apex_launchtest/apex_launchtest_main.py#L34
I think this is a result of the parametrization stuff getting grabbed while it was still WIP
test_runs = LoadTestsFromPythonModule(test_module, name=args.test_name + '.launch_tests') | ||
test_runs = LoadTestsFromPythonModule( | ||
test_module, name='{}.{}.launch_tests'.format( | ||
args.package_name, test_file_basename |
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.
Will this give silly names if --package-name
is not specified?
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.
Shouldn't, see line 102.
classname
attribute in thetestcase
element instead of jamming everything intoname
message
attribute forskipped
elementsSigned-off-by: Pete Baughman pete.baughman@apex.ai
I noticed some of our internal tooling was choking on apex_launchtest XML, so I've made the generated XML match pytest's XML more closely
Example pytest XML using same classnames and test names as the good_proc.test.py example, but with empty test methods:
Example launchtest XML:
It's not spot-on, but it's sufficient for our internal test reporting tooling. I'm open to feedback about what parts of the pytest junit XML are important.
This PR matches is my internal PR ApexAI/apex_rostest#41 on the Apex AI repo