667307 - Generate a JUnit style report for integration with a CI server

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

In order to take advantage of the reporting features of a CI server we should generate an XML report compatible with Ant's JUnit report.

I found the XSD here: http://windyroad.org/dl/Open%20Source/JUnit.xsd

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, how would an report look like? Do you have an URL to an example, probably what you already have for WebQA tests?

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

An example from the WebQA tests for Input, containing passes, fails, and skips (expected fails): http://qa-selenium.mv.mozilla.com:8080/job/input.stage/790/artifact/results.xml

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v1.0

Initial version for testing.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v1.1

Fixed issue discovered after running some tests.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v1.2

Improved name of test class.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave please attach an example output to this bug. Thanks.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 551912 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v1.2

>+    parser_options[("--junitxml",)] = dict(dest="junitxml_file")

We shouldn't care about the format. Just use 'junit' as option?

>+        self.junitxml_file = junitxml_file
>+        self.junitxml_file_count = 0

We should create a new class which handles that report format. If we wanna support multiple formats in the future we should also have a base class and stick everything into another module.

>+                result['filename'].partition('mozmill-tests')[2][1:].rpartition('.')[0].replace('.', '_'),

What is that doing in particular? Very hard to read and to understand without docs.

>+            elif len(result['fails']) > 0:
>+                fails += len(result['fails'])
>+                if len(result['fails']) == 1:
>+                    failure = result['fails'][0]
>+                    if 'exception' in failure:
>+                        message = failure['exception']['message']
>+                        body = failure['exception']['stack']
>+                    elif 'fail' in failure:
>+                        message = failure['fail']['message']
>+                        body = message
>+                    else:
>+                        message = 'Unknown failure.'
>+                        body = message
>+                else:
>+                    message = '%d failures' % len(result['fails'])
>+                    failures = []
>+                    for failure in result['fails']:
>+                        if 'exception' in failure:
>+                            failures.append('Message: "%s"<br/>Stack: %s' % (
>+                                failure['exception']['message'],
>+                                failure['exception']['stack']))
>+                        elif 'fail' in failure:
>+                            failures.append('Message: "%s"' % (
>+                                failure['fail']['message']))
>+                        else:
>+                            failures.append('Unknown failure.')
>+                    body = "<br/>".join(failures)

I don't think we need the if/else case here. Always build up an array and call join later. It shouldn't matter for that code how many failures happened in this test.

>+                testcase += '<failure message="%s">%s</failure>' % (message, body)
>+            testcase += '</testcase>'
>+            testcases.append(testcase)

I think we should define templates which we can fill in.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.0

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Comment on attachment 551912 [details] [diff] [review]
> Generate a JUnit style report for integration with a CI server. v1.2
> 
> >+    parser_options[("--junitxml",)] = dict(dest="junitxml_file")
> 
> We shouldn't care about the format. Just use 'junit' as option?

Done.

> >+        self.junitxml_file = junitxml_file
> >+        self.junitxml_file_count = 0
> 
> We should create a new class which handles that report format. If we wanna
> support multiple formats in the future we should also have a base class and
> stick everything into another module.

Done. Great suggestion, thanks!

> >+                result['filename'].partition('mozmill-tests')[2][1:].rpartition('.')[0].replace('.', '_'),
> 
> What is that doing in particular? Very hard to read and to understand
> without docs.

I've added comments. Basically it trims the 'classname' and replaces periods as these are interpreted as package hierarchy.

> >+            elif len(result['fails']) > 0:
> >+                fails += len(result['fails'])
> >+                if len(result['fails']) == 1:
> >+                    failure = result['fails'][0]
> >+                    if 'exception' in failure:
> >+                        message = failure['exception']['message']
> >+                        body = failure['exception']['stack']
> >+                    elif 'fail' in failure:
> >+                        message = failure['fail']['message']
> >+                        body = message
> >+                    else:
> >+                        message = 'Unknown failure.'
> >+                        body = message
> >+                else:
> >+                    message = '%d failures' % len(result['fails'])
> >+                    failures = []
> >+                    for failure in result['fails']:
> >+                        if 'exception' in failure:
> >+                            failures.append('Message: "%s"<br/>Stack: %s' % (
> >+                                failure['exception']['message'],
> >+                                failure['exception']['stack']))
> >+                        elif 'fail' in failure:
> >+                            failures.append('Message: "%s"' % (
> >+                                failure['fail']['message']))
> >+                        else:
> >+                            failures.append('Unknown failure.')
> >+                    body = "<br/>".join(failures)
> 
> I don't think we need the if/else case here. Always build up an array and
> call join later. It shouldn't matter for that code how many failures
> happened in this test.

I've removed some duplication here, but kept the if statement. If there is just one failure then the message should be the failure message, if there is more than one then the messages are in the body and the failure message indicates the number of failures.

> >+                testcase += '<failure message="%s">%s</failure>' % (message, body)
> >+            testcase += '</testcase>'
> >+            testcases.append(testcase)
> 
> I think we should define templates which we can fill in.

I'm not sure what you mean here. Can you give an example?

I also updated to use proper XML libraries for creating the report.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.1

Updated patch to apply cleanly.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Example report with both single and mutliple failures per test

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 554025 [details]
Example report with both single and mutliple failures per test

So a couple of questions before I want to check the patch itself.

><testsuite errors="0" failures="2" name="firefox-l10n" skips="0" tests="4" time="67">

Here 2 failures are mentioned while the real number of failures are 4. Also the number of tests are 2 and not 4. Probably you accidentally used the wrong assignments here?

>  <testcase classname="tests/l10n/testAccessKeys/test1" name="testPrefWindowAccessKeys" time="0">
>    <failure message="accessKey: d found in strings: [id: privacyDoNotTrackPrefs, label: Tell web sites I do not want to be tracked], [id: rememberDownloads, label: Remember download history]">

In your other example the message attribute always contains 'test failure'. I would kinda like to see it here too and that we only use the full exception message in the child node.

>  <testcase classname="tests/l10n/testCropped/test1" name="testPrefWindowCroppedElements" time="0">
>    <failure message="3 failures">

Can a testcase only contain a single failure node? If not we should really split all those failures into single sub entries.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 554025 [details]
> Example report with both single and mutliple failures per test
> 
> So a couple of questions before I want to check the patch itself.
> 
> ><testsuite errors="0" failures="2" name="firefox-l10n" skips="0" tests="4" time="67">
> 
> Here 2 failures are mentioned while the real number of failures are 4. Also
> the number of tests are 2 and not 4. Probably you accidentally used the
> wrong assignments here?

Failures in this report relates to the number of tests that are failing, not the number of checks failing within those tests. JUnit does not support the concept of multiple failures for a single test.

> >  <testcase classname="tests/l10n/testAccessKeys/test1" name="testPrefWindowAccessKeys" time="0">
> >    <failure message="accessKey: d found in strings: [id: privacyDoNotTrackPrefs, label: Tell web sites I do not want to be tracked], [id: rememberDownloads, label: Remember download history]">
> 
> In your other example the message attribute always contains 'test failure'.
> I would kinda like to see it here too and that we only use the full
> exception message in the child node.

This is the failure message as specified in the test. If there is only one failure then we treat it as JUnit report expects (failure message in the attribute, and full stack as a text child node). As this soft assertion doesn't have a full stack, the message is repeated as the text child node. The 'X failures' is a workaround to fit multiple failures per test into the report.

> >  <testcase classname="tests/l10n/testCropped/test1" name="testPrefWindowCroppedElements" time="0">
> >    <failure message="3 failures">
> 
> Can a testcase only contain a single failure node? If not we should really
> split all those failures into single sub entries.

Yes, as explained above.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #12)
> > ><testsuite errors="0" failures="2" name="firefox-l10n" skips="0" tests="4" time="67">
> > 
> > Here 2 failures are mentioned while the real number of failures are 4. Also
> > the number of tests are 2 and not 4. Probably you accidentally used the
> > wrong assignments here?
> 
> Failures in this report relates to the number of tests that are failing, not
> the number of checks failing within those tests. JUnit does not support the
> concept of multiple failures for a single test.

This is kinda bad. Have they never thought about that multiple failures could happen by using soft assertions? Any chance to get the XML Schema enhanced?

Further could we use errors for hard assertions and failures for soft assertions? As given by the XSD errors have to be used for exceptions, which is also the case for our asserts.

> This is the failure message as specified in the test. If there is only one
> failure then we treat it as JUnit report expects (failure message in the
> attribute, and full stack as a text child node). As this soft assertion
> doesn't have a full stack, the message is repeated as the text child node.
> The 'X failures' is a workaround to fit multiple failures per test into the
> report.

Soft assertions have a full stack available. I have implemented it last week on bug 680039. So we can already make use of it.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 554024 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.1

>+class Report(object):
>+

Please add a docstring with a description for this class.

>+        self.filename = filename
>+        self.file_count = 0

Can we call it index? It's at least used for it.

>+    def write(self):
>+        split_filename = self.filename.rpartition('.')
>+        unique_filename = '%s_%i.%s' % (split_filename[0], self.file_count, split_filename[2])
>+        file = open(unique_filename, 'w')
>+        file.write(self.report)
>+        file.close()
>+        self.file_count += 1

Please use a try/except here. Our test-run shouldn't abort early if we are not able to write the report file.

>+        time_start = datetime.fromtimestamp(time.mktime(
>+            time.strptime(raw_report['time_start'], datetime_format)))

Why can't we directly use datetime.strptime here?

>+        for result in raw_report['results']:
>+            class_name = result['filename'].partition('mozmill-tests')[2][1:]  # strip temporary and common path elements

You would like to split not at the 'mozmill-tests' level but for 'tests/%type%'. You can't expect that 'mozmill-tests' will be in the path.

>+            class_name = class_name.rpartition('.')[0]  # strip the file extension
>+            class_name = class_name.replace('.', '_')  # replace periods with underscore to avoid them being interpreted as package seperators

Means we have to do that because of a limitation in JUnit? What's the behavior when we leave in the extension and the dots?

>+            testcase_element.setAttribute("time", "0")  # add test duration

You can reference bug 583834. Once it is available we can implement that.

>+            if 'skipped' in result and result['skipped']:

We do not need have to check for result['skipped']. If 'skipped' is available we always have skipped the testcase.

>+                skipped_element = doc.createElement("skipped")

"skipped" is not a valid element given the XML Schema. Can you explain it a bit more?

>+            elif result['failed']:

Here you will have to check for 'failed' in result.

>+                for failure in result['fails']:
>+                    stack = None
>+                    if 'exception' in failure:
>+                        message = failure['exception']['message']
>+                        if 'stack' in failure['exception']:
>+                            stack = failure['exception']['stack']
>+                    elif 'fail' in failure:
>+                        message = failure['fail']['message']

The stack is available for hard and soft failures. So we can simplify this block a lot.

>+                    failures.append({'message' : message, 'stack' : stack})
>+                if len(failures) == 1:

Please add a blank line before the if statement. It's kinda hard to distinguish the different blocks here.

>+                    message = failure['message']
>+                    if failure['stack']:
>+                        body = failure['stack']
>+                    else:
>+                        body = message

Same as mentioned above. The stack is now always available.

>+                    message = '%d failures' % len(failures)
>+                    body = ''
>+                    for failure in failures:
>+                        body += 'Message: "%s"<br />' % failure['message']
>+                        if failure['stack']:
>+                            body += 'Stack: %s<br />' % failure['stack']

As talked on IRC please check if hard line wraps work here.

> class TestRun(object):
>+        self.custom_reports = []
[..]
>+        if junit_file:
>+            self.custom_reports.append(reports.JUnitReport(junit_file))

Please put those lines right next to each other.

>+        for report in self.custom_reports:
>+            report.generate(self._mozmill.mozmill.get_report())
>+            report.write()

Would have been nice if we could fold in the raw report in the constructor, so we wouldn't need the generate method. Therefore we should drop the custom_reports array, which we do not need at the moment anyway.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.2

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Comment on attachment 554024 [details] [diff] [review]
> Generate a JUnit style report for integration with a CI server. v2.1
> 
> >+class Report(object):
> >+
> 
> Please add a docstring with a description for this class.

Done.

> >+        self.filename = filename
> >+        self.file_count = 0
> 
> Can we call it index? It's at least used for it.

Done.

> >+    def write(self):
> >+        split_filename = self.filename.rpartition('.')
> >+        unique_filename = '%s_%i.%s' % (split_filename[0], self.file_count, split_filename[2])
> >+        file = open(unique_filename, 'w')
> >+        file.write(self.report)
> >+        file.close()
> >+        self.file_count += 1
> 
> Please use a try/except here. Our test-run shouldn't abort early if we are
> not able to write the report file.

Done.

> >+        time_start = datetime.fromtimestamp(time.mktime(
> >+            time.strptime(raw_report['time_start'], datetime_format)))
> 
> Why can't we directly use datetime.strptime here?

No reason, I'm still learning. Done. :)

> >+        for result in raw_report['results']:
> >+            class_name = result['filename'].partition('mozmill-tests')[2][1:]  # strip temporary and common path elements
> 
> You would like to split not at the 'mozmill-tests' level but for
> 'tests/%type%'. You can't expect that 'mozmill-tests' will be in the path.

I have improved this to split on test_path, however this also required me to make this value relative for the AddonsTestRun. I'm keen to hear your feedback on the changes.

> >+            class_name = class_name.rpartition('.')[0]  # strip the file extension
> >+            class_name = class_name.replace('.', '_')  # replace periods with underscore to avoid them being interpreted as package seperators
> 
> Means we have to do that because of a limitation in JUnit? What's the
> behavior when we leave in the extension and the dots?

As discussed in our 1-1, this is due to the way Java packages are hierarchical, using dots to separate each level. I have made some improvements here to reintroduce a hierarcy and better split the 'class' name from the test name.

> >+            testcase_element.setAttribute("time", "0")  # add test duration
> 
> You can reference bug 583834. Once it is available we can implement that.

Done.

> >+            if 'skipped' in result and result['skipped']:
> 
> We do not need have to check for result['skipped']. If 'skipped' is
> available we always have skipped the testcase.

My concern here is that if we ever have 'skipped' : False in the report then this would consider the test to have been skipped. By making sure it is both present and True we prevent that possibility.

> >+                skipped_element = doc.createElement("skipped")
> 
> "skipped" is not a valid element given the XML Schema. Can you explain it a
> bit more?

It's difficult to track down when skipped was added, however from what I can tell it may be something introduced by TestNG. In Jenkins the skipped tests do not affect the result of the testrun and are highlighted in the reports. This may not be the case for other CI servers.

> >+            elif result['failed']:
> 
> Here you will have to check for 'failed' in result.

Failed is always in result, however it's not always True.

> >+                for failure in result['fails']:
> >+                    stack = None
> >+                    if 'exception' in failure:
> >+                        message = failure['exception']['message']
> >+                        if 'stack' in failure['exception']:
> >+                            stack = failure['exception']['stack']
> >+                    elif 'fail' in failure:
> >+                        message = failure['fail']['message']
> 
> The stack is available for hard and soft failures. So we can simplify this
> block a lot.

Done.

> >+                    failures.append({'message' : message, 'stack' : stack})
> >+                if len(failures) == 1:
> 
> Please add a blank line before the if statement. It's kinda hard to
> distinguish the different blocks here.

Done.

> >+                    message = failure['message']
> >+                    if failure['stack']:
> >+                        body = failure['stack']
> >+                    else:
> >+                        body = message
> 
> Same as mentioned above. The stack is now always available.

Done.

> >+                    message = '%d failures' % len(failures)
> >+                    body = ''
> >+                    for failure in failures:
> >+                        body += 'Message: "%s"<br />' % failure['message']
> >+                        if failure['stack']:
> >+                            body += 'Stack: %s<br />' % failure['stack']
> 
> As talked on IRC please check if hard line wraps work here.

Done.

> > class TestRun(object):
> >+        self.custom_reports = []
> [..]
> >+        if junit_file:
> >+            self.custom_reports.append(reports.JUnitReport(junit_file))
> 
> Please put those lines right next to each other.

No longer relevant - see next comment.

> >+        for report in self.custom_reports:
> >+            report.generate(self._mozmill.mozmill.get_report())
> >+            report.write()
> 
> Would have been nice if we could fold in the raw report in the constructor,
> so we wouldn't need the generate method. Therefore we should drop the
> custom_reports array, which we do not need at the moment anyway.

I have modified the constructor, and am interested to hear your feedback.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 558655 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.2

Dropping to f? as I've noticed a few issues with this latest patch, but am still keen to hear feedback for some of the latest changes.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.3

New patch with a few things fixed:

Fixed index for multiple files
Fixed reports with unicode messages (localisations)
Fixed failures without a list of fails
Fixed reports without stack traces (disconnect errors)

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #15)
> > >+        for result in raw_report['results']:
> > >+            class_name = result['filename'].partition('mozmill-tests')[2][1:]  # strip temporary and common path elements
> > 
> > You would like to split not at the 'mozmill-tests' level but for
> > 'tests/%type%'. You can't expect that 'mozmill-tests' will be in the path.
> 
> I have improved this to split on test_path, however this also required me to
> make this value relative for the AddonsTestRun. I'm keen to hear your
> feedback on the changes.

Can you give me an actual example report based on the new patch? I would kinda appreciate that, and which would also help me with the review.

> > >+                skipped_element = doc.createElement("skipped")
> > 
> > "skipped" is not a valid element given the XML Schema. Can you explain it a
> > bit more?
> 
> It's difficult to track down when skipped was added, however from what I can
> tell it may be something introduced by TestNG. In Jenkins the skipped tests
> do not affect the result of the testrun and are highlighted in the reports.
> This may not be the case for other CI servers.

So does it mean that there is no updated XSD is available, which can be used to check the created report? The link you have given me earlier on this bug seems to be outdated.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Can you give me an actual example report based on the new patch? I would
> kinda appreciate that, and which would also help me with the review.

I will attach a l10n report. I'm not sure if there are specific cases you'd like to see in the report. They're very simple to generate with this patch though so I'd recommend doing that.

> > It's difficult to track down when skipped was added, however from what I can
> > tell it may be something introduced by TestNG. In Jenkins the skipped tests
> > do not affect the result of the testrun and are highlighted in the reports.
> > This may not be the case for other CI servers.
> 
> So does it mean that there is no updated XSD is available, which can be used
> to check the created report? The link you have given me earlier on this bug
> seems to be outdated.

There is no official XSD that I am aware of - I believe the one I linked to was based on the original Ant code that generated the report, and it should not be strictly applied to the resulting reports from this patch.

This is likely due to the fact that there are a few variations on the report. As I mentioned, the skipped is something I believe was added specifically for TestNG. Not all continuous integration tools will necessarily support the JUnit report, and I'm sure they'll have different interpretations.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Example report (l10n)

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 559183 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.3

Sorry, that it has been taken a bit longer. But here finally me comments to the changes, which are looking good so far but could need some more improvements and changes.

>+++ b/libs/reports.py

nit: In case of all the other modules we are using no plurals. I would like to see us consistent here.

>+    def __init__(self, raw_report, filename):
>+        self.raw_report = raw_report
>+        self.filename = filename
>+        self.report = ""
>+        self._generate()
>+        self._write()

Will there ever be a case that you will have to call _generate alone? Right now I do not see a situation. I feel that the write method should simply call _generate internally, whereby the latter method returns the created custom report string instead of putting it as a member to the class. It would make things less complex. 

>+    def _write(self):
>+        try:
>+            file = open(self.filename, 'w')
>+            file.write(self.report)
>+            file.close()
>+        except IOError, e:
>+            raise e

In the above case you are re-raising the exception. It's the same behavior as without the try/except code. We have to write out an error to stderr and continue by ignoring the failure.

>+        datetime_format = '%Y-%m-%dT%H:%M:%SZ'

Lets move this up to a global constant. That way other future custom reports can re-use it. As I can see it's the standard ISO format so I wouldn't expect any problems.

>+        testsuite_element.setAttribute("time", str(abs(time_start - time_end).seconds))

Why do you substract the end time from the start time? Should be the other way around so we can also get rid of the abs call.

>+            class_name = filename[filename.find(self.test_path):]  # strip temporary and common path elements

I still think that we should not start with the 'tests' folder as the top folder for the test path. Instead we should use the report-type stripped off the 'firefox-' prefix to get the path of test module. In case of l10n tests I would not include 'tests.l10n' in the class_name because it's redundant information given the name of the test-suite.

>+            class_name = class_name.rpartition('.')[0]  # strip the file extension
>+            class_name = class_name.replace('.', '_')  # replace periods with underscore to avoid them being interpreted as package seperators
>+            class_name = class_name.replace(os.path.sep, '.')  # replace periods with underscore to avoid them being interpreted as package seperators

The comment on the last line doesn't match up with the code. Is the comment wrong or the code?

>+                for failure in result_failures:
>+                    # If the failure is a dict then return the appropriate exception/failure item or return an empty dict
>+                    failure_data = isinstance(failure, dict) and 'exception' in failure and failure['exception'] or \
>+                                   isinstance(failure, dict) and 'fail' in failure and failure['fail'] or {}

I don't think that we necessarily have to check for the dict type here. Whether we have no failure or it's a dict.

>+                    message = 'message' in failure_data and failure_data['message'] or 'Unknown failure.'
>+                    stack = 'stack' in failure_data and failure_data['stack'] or 'Stack unavailable.'

failure_data is a Boolean. So how do those two lines work? Shouldn't it get the data from failure? Also a message and stack will always be available, so why do we have to check if both are present?


>+                if len(failures) == 1:
>+                    failure = failures[0]
>+                    message = str(failure['message'].encode('utf-8'))
>+                    body = str(failure['stack'])

This can become '(body, message) = failures[0].values()'. Also the encoding should happen in the for loop above when assigning the message.

>+                else:
>+                    message = '%d failures' % len(failures)
>+                    body = '\n\n'.join(['Message: %s\nStack: %s' % (failure['message'], failure['stack']) for failure in failures])

... That way you will not have to use .encode() here again, which is missing here btw. 

>+++ b/libs/testrun.py
>+        self.custom_report_index = 0

Lets take a more general name so we can also use the index for the log file and the report file later on bug 626715.

>+    def _generate_custom_reports(self):

We create a single report file here. So please use the singular. 

>+    def _get_unique_report_filename(self, filename):
>+        split_filename = filename.rpartition('.')
>+        return '%s_%i.%s' % (split_filename[0], self.custom_report_index, split_filename[2])

I would remove report from the method's name, so we can also use it for the log file later.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.4

(In reply to Henrik Skupin (:whimboo) from comment #21)
> >+++ b/libs/reports.py
> 
> nit: In case of all the other modules we are using no plurals. I would like
> to see us consistent here.

Done.

> >+    def __init__(self, raw_report, filename):
> >+        self.raw_report = raw_report
> >+        self.filename = filename
> >+        self.report = ""
> >+        self._generate()
> >+        self._write()
> 
> Will there ever be a case that you will have to call _generate alone? Right
> now I do not see a situation. I feel that the write method should simply
> call _generate internally, whereby the latter method returns the created
> custom report string instead of putting it as a member to the class. It
> would make things less complex. 

Done.

> >+    def _write(self):
> >+        try:
> >+            file = open(self.filename, 'w')
> >+            file.write(self.report)
> >+            file.close()
> >+        except IOError, e:
> >+            raise e
> 
> In the above case you are re-raising the exception. It's the same behavior
> as without the try/except code. We have to write out an error to stderr and
> continue by ignoring the failure.

My mistake, sorry. I am now writing a message to stderr.

> >+        datetime_format = '%Y-%m-%dT%H:%M:%SZ'
> 
> Lets move this up to a global constant. That way other future custom reports
> can re-use it. As I can see it's the standard ISO format so I wouldn't
> expect any problems.

Done.

> >+        testsuite_element.setAttribute("time", str(abs(time_start - time_end).seconds))
> 
> Why do you substract the end time from the start time? Should be the other
> way around so we can also get rid of the abs call.

Done.

> >+            class_name = filename[filename.find(self.test_path):]  # strip temporary and common path elements
> 
> I still think that we should not start with the 'tests' folder as the top
> folder for the test path. Instead we should use the report-type stripped off
> the 'firefox-' prefix to get the path of test module. In case of l10n tests
> I would not include 'tests.l10n' in the class_name because it's redundant
> information given the name of the test-suite.

Done.

> >+            class_name = class_name.rpartition('.')[0]  # strip the file extension
> >+            class_name = class_name.replace('.', '_')  # replace periods with underscore to avoid them being interpreted as package seperators
> >+            class_name = class_name.replace(os.path.sep, '.')  # replace periods with underscore to avoid them being interpreted as package seperators
> 
> The comment on the last line doesn't match up with the code. Is the comment
> wrong or the code?

The comment was incorrect. I have fixed it.

> >+                for failure in result_failures:
> >+                    # If the failure is a dict then return the appropriate exception/failure item or return an empty dict
> >+                    failure_data = isinstance(failure, dict) and 'exception' in failure and failure['exception'] or \
> >+                                   isinstance(failure, dict) and 'fail' in failure and failure['fail'] or {}
> 
> I don't think that we necessarily have to check for the dict type here.
> Whether we have no failure or it's a dict.

I found a case where the value of failure is an integer. I believe it was on a disconnect.

> >+                    message = 'message' in failure_data and failure_data['message'] or 'Unknown failure.'
> >+                    stack = 'stack' in failure_data and failure_data['stack'] or 'Stack unavailable.'
> 
> failure_data is a Boolean. So how do those two lines work? Shouldn't it get
> the data from failure? Also a message and stack will always be available, so
> why do we have to check if both are present?

I'm not sure why you think failure_data is a Boolean. It's a dictionary. Message is not available in some cases, where you have 'Unknown Test' as we currently have for one of the addons. Stack is unavailable in similar situations.

> 
> >+                if len(failures) == 1:
> >+                    failure = failures[0]
> >+                    message = str(failure['message'].encode('utf-8'))
> >+                    body = str(failure['stack'])
> 
> This can become '(body, message) = failures[0].values()'. Also the encoding
> should happen in the for loop above when assigning the message.

Thanks, done. I have also driven myself close to insanity trying to work out encodings. I have now removed this as it's working without, even when running against the German localisation with failures.

> >+                else:
> >+                    message = '%d failures' % len(failures)
> >+                    body = '\n\n'.join(['Message: %s\nStack: %s' % (failure['message'], failure['stack']) for failure in failures])
> 
> ... That way you will not have to use .encode() here again, which is missing
> here btw. 

See previous comment.

> >+++ b/libs/testrun.py
> >+        self.custom_report_index = 0
> 
> Lets take a more general name so we can also use the index for the log file
> and the report file later on bug 626715.

Renamed to testrun_index.

> >+    def _generate_custom_reports(self):
> 
> We create a single report file here. So please use the singular. 

In future other custom reports would be included here, however I have renamed due to the current presence of just one custom report.

> >+    def _get_unique_report_filename(self, filename):
> >+        split_filename = filename.rpartition('.')
> >+        return '%s_%i.%s' % (split_filename[0], self.custom_report_index, split_filename[2])
> 
> I would remove report from the method's name, so we can also use it for the
> log file later.

Done.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 560346 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.4

So we are getting closer. Looks mostly good, but still need some tweaks for special tasks.

>+    """ Class to customise the Mozmill report. """

nit: 'customize'

>+    def _generate(self):
>+        return raw_report

Please add a comment that this method has to be overwritten by a sub class.

>+    def _write(self):
>+        report = self._generate()
>+        try:
>+            file = open(self.filename, 'w')
>+            file.write(report)
>+            file.close()
>+        except Exception, e:
>+            sys.stderr.write('Failed to write report to file: %s\n' % e)

Why have you changed this from IOError to Exception? It will still be an IOError if something fails here, so that way you do not get the exception. Also it should be read as 'except IOError as e'.

>+            test_path = os.path.sep.join(['tests', report_type.split('firefox-')[1]])

Just use os.path.join here and specifying a list of strings as parameters. Also please rename the variable to e.g. root_path
so it's more understandable that this is the root path where the tests are contained in.

>+            class_name = filename.partition(test_path)[2].lstrip(os.path.sep)  # strip temporary and common path elements

There is no need for the lstrip part with the 'os.path.sep' addition. To be really on the safe side we should convert all double blackslashes to slashes before doing any action on the filename. Only that will allow us a cross-platform handling.

>+            class_name = class_name.rpartition('.')[0]  # strip the file extension

It's more obvious and probably more stable here when you would use 'os.path.splitext(class_name)[0].

>+            class_name = class_name.replace('.', '_')  # replace periods with underscore to avoid them being interpreted as package seperators

So why is that necessary at this point? There shouldn't be any dots anymore in the current path once we stripped of the extension part and the unwanted system level path.

>+            class_name = class_name.replace(os.path.sep, '.')  # replace path separators with periods to give implied package hierarchy

With the above change you could always use '/' as source now.

Also please put the comments in separate lines so we do not necessarily extend the 80 char limit.

>+                # If result['fails'] is not a list, make it a list of one
>+                result_failures = result['fails']
>+                if not isinstance(result_failures, list):
>+                    result_failures = [result_failures]

result['fails'] is always a list in the JSON object: "fails": [ ]. When do we need that special casing exactly? I could only see a framework bug here.

>+                for failure in result_failures:
>+                    # If the failure is a dict then return the appropriate exception/failure item or return an empty dict
>+                    failure_data = isinstance(failure, dict) and 'exception' in failure and failure['exception'] or \
>+                                   isinstance(failure, dict) and 'fail' in failure and failure['fail'] or {}
[..]
>> I found a case where the value of failure is an integer. I believe it was on a disconnect.

Same here. I would need an example when this happens. I really can't see a situation like that.

>+                    message = 'message' in failure_data and failure_data['message'] or 'Unknown failure.'
>+                    stack = 'stack' in failure_data and failure_data['stack'] or 'Stack unavailable.'

Please use: failure_data.get('message', %default_value)

>+        print 'returning xml'

Please remove this line.

>+        self.testrun_index = 0

The class is called testrun. So we should at least name it to test_index to make a distinction.

>+    def _get_unique_filename(self, filename):
>+        split_filename = filename.rpartition('.')
>+        return '%s_%i.%s' % (split_filename[0], self.testrun_index, split_filename[2])

Same as above. Please use the splitext method.

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

An example of a report that contains "fails" : 1
http://mozmill-release.brasstacks.mozilla.com/db/10a26c9f0103d541c88ebfa5ccde74ac

Like you say, this may be a framework bug, however I don't see the harm in safeguarding my code from bad data.

I have a new patch ready for testing and will upload once I'm happy with it.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.5

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Comment on attachment 560346 [details] [diff] [review]
> Generate a JUnit style report for integration with a CI server. v2.4
> 
> So we are getting closer. Looks mostly good, but still need some tweaks for
> special tasks.
> 
> >+    """ Class to customise the Mozmill report. """
> 
> nit: 'customize'

Done. Is there a policy on using en-US over en-GB?

> >+    def _generate(self):
> >+        return raw_report
> 
> Please add a comment that this method has to be overwritten by a sub class.

Done.

> >+    def _write(self):
> >+        report = self._generate()
> >+        try:
> >+            file = open(self.filename, 'w')
> >+            file.write(report)
> >+            file.close()
> >+        except Exception, e:
> >+            sys.stderr.write('Failed to write report to file: %s\n' % e)
> 
> Why have you changed this from IOError to Exception? It will still be an
> IOError if something fails here, so that way you do not get the exception.
> Also it should be read as 'except IOError as e'.

Isn't IOError a subclass of Exception? I have updated. Also, the 'except X as Y' is not the prevailing style used in mozmill-automation.

> >+            test_path = os.path.sep.join(['tests', report_type.split('firefox-')[1]])
> 
> Just use os.path.join here and specifying a list of strings as parameters.
> Also please rename the variable to e.g. root_path
> so it's more understandable that this is the root path where the tests are
> contained in.

Done.

> >+            class_name = filename.partition(test_path)[2].lstrip(os.path.sep)  # strip temporary and common path elements
> 
> There is no need for the lstrip part with the 'os.path.sep' addition. To be
> really on the safe side we should convert all double blackslashes to slashes
> before doing any action on the filename. Only that will allow us a
> cross-platform handling.

I don't understand your comment here. Without the lstrip we will have a trailing path separator... I'm also not clear why we'd want to convert double backslashes to slashes. Isn't os.path.sep a safe cross-platform way of handling path separators?

> >+            class_name = class_name.rpartition('.')[0]  # strip the file extension
> 
> It's more obvious and probably more stable here when you would use
> 'os.path.splitext(class_name)[0].

Thanks, I wasn't aware of that one. Done.

> >+            class_name = class_name.replace('.', '_')  # replace periods with underscore to avoid them being interpreted as package seperators
> 
> So why is that necessary at this point? There shouldn't be any dots anymore
> in the current path once we stripped of the extension part and the unwanted
> system level path.

Because some folders contain periods. For example [e-mail]. We don't want these periods to be interpreted as hierarchies.

> >+            class_name = class_name.replace(os.path.sep, '.')  # replace path separators with periods to give implied package hierarchy
> 
> With the above change you could always use '/' as source now.

I assume this goes back to the path separator comment from above. Please clarify.

> Also please put the comments in separate lines so we do not necessarily
> extend the 80 char limit.

Done. I have put the comments above the lines of code they apply to and added some new lines for readability.

> >+                # If result['fails'] is not a list, make it a list of one
> >+                result_failures = result['fails']
> >+                if not isinstance(result_failures, list):
> >+                    result_failures = [result_failures]
> 
> result['fails'] is always a list in the JSON object: "fails": [ ]. When do
> we need that special casing exactly? I could only see a framework bug here.

See comment 24 for an example of where this is necessary.

> >+                for failure in result_failures:
> >+                    # If the failure is a dict then return the appropriate exception/failure item or return an empty dict
> >+                    failure_data = isinstance(failure, dict) and 'exception' in failure and failure['exception'] or \
> >+                                   isinstance(failure, dict) and 'fail' in failure and failure['fail'] or {}
> [..]
> >> I found a case where the value of failure is an integer. I believe it was on a disconnect.
> 
> Same here. I would need an example when this happens. I really can't see a
> situation like that.

As above.

> >+                    message = 'message' in failure_data and failure_data['message'] or 'Unknown failure.'
> >+                    stack = 'stack' in failure_data and failure_data['stack'] or 'Stack unavailable.'
> 
> Please use: failure_data.get('message', %default_value)
> 
> >+        print 'returning xml'

Oops! Done.

> Please remove this line.
> 
> >+        self.testrun_index = 0
> 
> The class is called testrun. So we should at least name it to test_index to
> make a distinction.

Done, although this now could be misread as the count increments with each testrun and not each test.

> >+    def _get_unique_filename(self, filename):
> >+        split_filename = filename.rpartition('.')
> >+        return '%s_%i.%s' % (split_filename[0], self.testrun_index, split_filename[2])
> 
> Same as above. Please use the splitext method.

Done.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #24)
> An example of a report that contains "fails" : 1
> http://mozmill-release.brasstacks.mozilla.com/db/
> 10a26c9f0103d541c88ebfa5ccde74ac
> 
> Like you say, this may be a framework bug, however I don't see the harm in
> safeguarding my code from bad data.

Please file this as Mozmill bug. In this case it happened because a referenced XPCOM component wasn't able to import, and Mozmill failed to execute the test file. Thanks.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 561162 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.5

(In reply to Dave Hunt (:davehunt) from comment #25)
> > >+    """ Class to customise the Mozmill report. """
> > 
> > nit: 'customize'
> 
> Done. Is there a policy on using en-US over en-GB?

You should ask the other way around. We have started with en-US so we should stick with it.

> > Why have you changed this from IOError to Exception? It will still be an
> > IOError if something fails here, so that way you do not get the exception.
> > Also it should be read as 'except IOError as e'.
> 
> Isn't IOError a subclass of Exception? I have updated. Also, the 'except X
> as Y' is not the prevailing style used in mozmill-automation.

It is but you do not want to catch any exception here. We only want to detect IO errors. Also 'as' has to be used, and the other places have to be updated. It's not about style, it's about correctness and visible deprecation messages.

>>+            # strip temporary and common path elements
>>+            class_name = filename.partition(root_path)[2].lstrip(os.path.sep)
>>[..]
>>+            # replace path separators with periods to give implied package hierarchy
>>+            class_name = class_name.replace(os.path.sep, '.')
>
> I don't understand your comment here. Without the lstrip we will have a
> trailing path separator... I'm also not clear why we'd want to convert
> double backslashes to slashes. Isn't os.path.sep a safe cross-platform way
> of handling path separators?

I'm not sure about cygwin and what's the default there. Is it '/' or '\'? We would simply be on the safe side and have unique path representations before starting to separate the fragments. Re: lstrip I meant you can use a simple slash then. I know it is necessary. Same for the last replace.

>>+            # replace periods with underscore to avoid them being interpreted as package seperators
>>+            class_name = class_name.replace('.', '_')
>
> Because some folders contain periods. For example [e-mail]. We
> don't want these periods to be interpreted as hierarchies.

Ah, right. Missed that. Thanks!

>+                    failure_data = isinstance(failure, dict) and 'exception' in failure and failure['exception'] or \
>+                                   isinstance(failure, dict) and 'fail' in failure and failure['fail'] or {}

nit: we could strip on of the isinstance checks.

>>+        self.test_index = 0
> 
> Done, although this now could be misread as the count increments with each
> testrun and not each test.

So we increment this value between any kind of test-run, e.g. non-restart, restart, endurance, l10n... right? For each binary we restart by zero? Seems like we have to think about a better naming.

>> -return '%s_%i.%s' % (split_filename[0], self.testrun_index, split_filename[2])
>> +return '{0[0]}_{1}{0[1]}'.format(os.path.splitext(filename), self.test_index)

Why have you changed this part to that hardly to read format string? I would even prefer to see named placeholders like %(FILENAME)s. it was ok to have a temporary variable here.

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.6

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Comment on attachment 561162 [details] [diff] [review] [diff] [details] [review]
> Generate a JUnit style report for integration with a CI server. v2.5
> 
> >>+            # strip temporary and common path elements
> >>+            class_name = filename.partition(root_path)[2].lstrip(os.path.sep)
> >>[..]
> >>+            # replace path separators with periods to give implied package hierarchy
> >>+            class_name = class_name.replace(os.path.sep, '.')
> >
> > I don't understand your comment here. Without the lstrip we will have a
> > trailing path separator... I'm also not clear why we'd want to convert
> > double backslashes to slashes. Isn't os.path.sep a safe cross-platform way
> > of handling path separators?
> 
> I'm not sure about cygwin and what's the default there. Is it '/' or '\'? We
> would simply be on the safe side and have unique path representations before
> starting to separate the fragments. Re: lstrip I meant you can use a simple
> slash then. I know it is necessary. Same for the last replace.

Sorry, I'm still not clear on what you want me to do here -- please clarify.

> >+                    failure_data = isinstance(failure, dict) and 'exception' in failure and failure['exception'] or \
> >+                                   isinstance(failure, dict) and 'fail' in failure and failure['fail'] or {}
> 
> nit: we could strip on of the isinstance checks.

Done.

> >>+        self.test_index = 0
> > 
> > Done, although this now could be misread as the count increments with each
> > testrun and not each test.
> 
> So we increment this value between any kind of test-run, e.g. non-restart,
> restart, endurance, l10n... right? For each binary we restart by zero? Seems
> like we have to think about a better naming.

It's incremented every time run_tests is called, and reset on initialisation of a TestRun object. I thought testrun_index would make the most sense but I'm happy with whatever you want to call it.

> >> -return '%s_%i.%s' % (split_filename[0], self.testrun_index, split_filename[2])
> >> +return '{0[0]}_{1}{0[1]}'.format(os.path.splitext(filename), self.test_index)
> 
> Why have you changed this part to that hardly to read format string? I would
> even prefer to see named placeholders like %(FILENAME)s. it was ok to have a
> temporary variable here.

I agree that my change wasn't readable, however I wasn't keen on the temporary value either. I have reverted back to the previous style.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 562490 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.6

>+            filename = result['filename']
>+            root_path = os.path.join('tests', report_type.split('firefox-')[1])
>+
>+            # strip temporary and common path elements
>+            class_name = filename.partition(root_path)[2].lstrip(os.path.sep)
>
> > I'm not sure about cygwin and what's the default there. Is it '/' or '\'? We
> > would simply be on the safe side and have unique path representations before
> > starting to separate the fragments. Re: lstrip I meant you can use a simple
> > slash then. I know it is necessary. Same for the last replace.
> 
> Sorry, I'm still not clear on what you want me to do here -- please clarify.

As I have mentioned already earlier on that bug, replace the backslashes at first with slashes. Then we have unique test module paths and can always refer to slashes and don't have to use os.path.sep.

>+                    # If the failure is a dict then return the appropriate exception/failure item or return an empty dict
>+                    failure_data = isinstance(failure, dict) and ( \
>+                        'exception' in failure and failure['exception'] or \
>+                        'fail' in failure and failure['fail']) or {}

Why do we need the backslashes at the end of each line? It's not a string so it shouldn't break without them. Also please align the last two lines 4 spaces to the right of isinstance.

>+        self.test_index = 0
>
> > So we increment this value between any kind of test-run, e.g. non-restart,
> > restart, endurance, l10n... right? For each binary we restart by zero? Seems
> > like we have to think about a better naming.
> 
> It's incremented every time run_tests is called, and reset on initialisation
> of a TestRun object. I thought testrun_index would make the most sense but
> I'm happy with whatever you want to call it.

Ok, don't let us hold off on that. Please rename it back to testrun_index for now. You are right, it's more clear that way. I don't have anything else to propose right now. 

>+    def _get_unique_filename(self, filename):
>+        split_filename = os.path.splitext(filename)

You can do '(basename, ext) = os.path.splitext(filename)'. That way it will make the second line a bit shorter.

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.7

(In reply to Henrik Skupin (:whimboo) from comment #29)
> 
> As I have mentioned already earlier on that bug, replace the backslashes at
> first with slashes. Then we have unique test module paths and can always
> refer to slashes and don't have to use os.path.sep.

I hope I've understood what you wanted here. Please correct me if I'm wrong.

> >+                    # If the failure is a dict then return the appropriate exception/failure item or return an empty dict
> >+                    failure_data = isinstance(failure, dict) and ( \
> >+                        'exception' in failure and failure['exception'] or \
> >+                        'fail' in failure and failure['fail']) or {}
> 
> Why do we need the backslashes at the end of each line? It's not a string so
> it shouldn't break without them. Also please align the last two lines 4
> spaces to the right of isinstance.

Done.

> >+        self.test_index = 0
> > 
> > It's incremented every time run_tests is called, and reset on initialisation
> > of a TestRun object. I thought testrun_index would make the most sense but
> > I'm happy with whatever you want to call it.
> 
> Ok, don't let us hold off on that. Please rename it back to testrun_index
> for now. You are right, it's more clear that way. I don't have anything else
> to propose right now. 

Done.

> >+    def _get_unique_filename(self, filename):
> >+        split_filename = os.path.splitext(filename)
> 
> You can do '(basename, ext) = os.path.splitext(filename)'. That way it will
> make the second line a bit shorter.

Done.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 562597 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.7

>+            root_path = os.path.join('tests', report_type.split('firefox-')[1])
>+
>+            # strip temporary and common path elements
>+            class_name = filename.partition(root_path)[2]
>+            
>+            # replace backslashes with forward slashes, and strip trailing forward slash
>+            class_name = class_name.replace('\\', '/').lstrip('/')

The replacement of backslashes has to happen before you partition the filename, otherwise it will fail due to different path separators. Given that I missed to note that we can't use os.path.join for the root_path initialization. Just change it to a normal string operation and use a slash as separator. Then it will be fully cross-platform compliant.

Comment 32

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.8

(In reply to Henrik Skupin (:whimboo) from comment #31)
> Comment on attachment 562597 [details] [diff] [review] [diff] [details] [review]
> Generate a JUnit style report for integration with a CI server. v2.7
> 
> >+            root_path = os.path.join('tests', report_type.split('firefox-')[1])
> >+
> >+            # strip temporary and common path elements
> >+            class_name = filename.partition(root_path)[2]
> >+            
> >+            # replace backslashes with forward slashes, and strip trailing forward slash
> >+            class_name = class_name.replace('\\', '/').lstrip('/')
> 
> The replacement of backslashes has to happen before you partition the
> filename, otherwise it will fail due to different path separators. Given
> that I missed to note that we can't use os.path.join for the root_path
> initialization. Just change it to a normal string operation and use a slash
> as separator. Then it will be fully cross-platform compliant.

Done.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 562706 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.8

>+        for result in self.raw_report['results']:
>+            filename = result['filename']
>+            root_path = '/'.join('tests', report_type.split('firefox-')[1])
>+
>+            # replace backslashes with forward slashes
>+            class_name = class_name.replace('\\', '/')
>+
>+            # strip temporary and common path elements, and strip trailing forward slash
>+            class_name = filename.partition(root_path)[2].lstrip('/')

Class name is not defined for the replace call. It has to be filename. Please always test your patches even for those small changes.

Comment 34

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Generate a JUnit style report for integration with a CI server. v2.9

(In reply to Henrik Skupin (:whimboo) from comment #33)
> Comment on attachment 562706 [details] [diff] [review] [diff] [details] [review]
> Generate a JUnit style report for integration with a CI server. v2.8
> 
> >+        for result in self.raw_report['results']:
> >+            filename = result['filename']
> >+            root_path = '/'.join('tests', report_type.split('firefox-')[1])
> >+
> >+            # replace backslashes with forward slashes
> >+            class_name = class_name.replace('\\', '/')
> >+
> >+            # strip temporary and common path elements, and strip trailing forward slash
> >+            class_name = filename.partition(root_path)[2].lstrip('/')
> 
> Class name is not defined for the replace call. It has to be filename.
> Please always test your patches even for those small changes.

Of course the one time I don't test is the time it fails! Sorry about that, I'll be much more careful in future.

Note that I have also fixed an encoding issue in this patch, which is encountered when using locales with extended character sets.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 562774 [details] [diff] [review]
Generate a JUnit style report for integration with a CI server. v2.9

Looks good. Thanks for also catching the encoding issue. I assume it's well tested now on your side. So please land it.

Comment 36

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks Henrik!

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/12d178d32e77