1065406 - [mozlog] Split test class and name for XUnit formatter

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

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

The XUnit and HTML formatters do not currently represent the test class and names well when compared to the Marionette test runner reports.

Marionette reports:
Class: test_prefs.TestPrefs
Name: test_get_set_bool_pref_from_within_an_app

mozlog HTML formatter:
Class: <empty>
Name: test_prefs.py TestPrefs.test_get_set_bool_pref_from_within_an_app

mozlog XUnit formatter:
Class: test_prefs_py TestPrefs_test_get_set_bool_pref_from_within_an_app
Name: test_prefs_py TestPrefs_test_get_set_bool_pref_from_within_an_app

If there's no good way to get a class/name then we don't need this for the HTML report as we just need a way to identify the test. For the XUnit report we should have some unambiguous value for both class and name, even if it's not a perfect representation of the test.

Comment 1

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

Hey Julien, are you interested in working on this?

Comment 2

[Julien Pagès (:parkouss)]

Hi Dave,

Yep, I'd like it!

Comment 3

[Julien Pagès (:parkouss)]

Well, this is going to be not so simple. Here is what I found so far:

 - Marionette tests implement the id() method of unittest.TestCase subclasses to return a "concatenation" like of module name and object name: http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette_test.py#340

 - moztest use this value to call the logger.test_start method: http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/moztest/moztest/adapters/unit.py#45

 - and finally mozlog got this string value - here we are. :)

The thing is that we could use regex in mozlog formatters, to extract information. But this feels wrong, as the string format is defined in marionette, and used in mozlog - but I suspect that mozlog should have nothing to do with marionette, even for a "simple" regexp use;

A possible better implementation would be to add structured information for the test_add message. If I understand well, something like:

@log_action(TestId("test"),
            Unicode("classname"), optionnal=True),
            Unicode(filename), optionnal=True))
    def test_start(self, data):
        ...

Then make these information available in marionette (with some methods on CommonTestCase) and in moztest test_start call fill the data if available. Finally formatters could use them.

What do you think ?

Comment 4

[James Graham [:jgraham]]

Yeah, so I think that filename clearly makes sense. I wonder if there's something slightly more generic than classname that applies to more types of tests e.g. xpcshell. Chris: what do you think?

Comment 5

[Chris Manchester (limited bugmail, email directly)]

(In reply to James Graham [:jgraham] from comment #4)
> Yeah, so I think that filename clearly makes sense. I wonder if there's
> something slightly more generic than classname that applies to more types of
> tests e.g. xpcshell. Chris: what do you think?

Identifying tests is still fairly harness specific, and the test ids are the way they are for treeherder compatibility (so I don't expect we'll have a great answer for xpcshell as a result of fixing this bug). This bug seems about getting data into the format expected by xunit, so I don't see a problem adding optional fields (or maybe putting things in "extra") to test_start that reflect this. The proposal in comment 3 seems to do this well (although I don't think adding regex to mozlog will get a lot of traction).

Comment 6

[James Graham [:jgraham]]

Right, I agree. I was just wondering if there's something slightly more generic than (file, classname) that would solve more usecases, rather than later adding functionname, hashname, etc. to cover cases that aren't strictly classes.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

A classname seems like a subtest, but we aren't using them here because they already mean something incompatible for other formatters. I think we might better off catering to the model of the target format directly in this case.

Comment 8

[James Graham [:jgraham]]

OK

Comment 9

[Julien Pagès (:parkouss)]

Attachment: Split test class and name for XUnit formatter

Well I choosed to use the "extra" as stated in comment 5 (I needed class name and method name in fact).

Xml output now looks like:

<?xml version="1.0" encoding="utf8"?>
<testsuite errors="0" failures="0" skips="0" tests="3" time="0.69">
	<testcase classname="test_click.TestClick" name="test_click" time="0.24"/>
	<testcase classname="test_click.TestClick" name="test_clicking_a_link_made_up_of_numbers_is_handled_correctly" time="0.28"/>
	<testcase classname="test_click.TestClick" name="test_clicking_an_element_that_is_not_displayed_raises" time="0.14"/>
</testsuite>

And same formatting in the html formatter. By the way, this is not related to this bug but html formatter use a dependency 'py.xml' that is not installed by default and the use of the html formatter raise an exception accordingly.

A simple 'pip install py' resolve this (but it takes me a couple of minutes to figure it), and it seems that it is not a big package. Maybe it could be good to add this dependency in the mozlog setup.py file, or at least raise a better exception to indicate what to install ?

I can create a bug entry if you like the idea and that is not already reported somewhere. :)

Comment 10

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

(In reply to Julien Pagès from comment #9)
> And same formatting in the html formatter. By the way, this is not related
> to this bug but html formatter use a dependency 'py.xml' that is not
> installed by default and the use of the html formatter raise an exception
> accordingly.
> 
> A simple 'pip install py' resolve this (but it takes me a couple of minutes
> to figure it), and it seems that it is not a big package. Maybe it could be
> good to add this dependency in the mozlog setup.py file, or at least raise a
> better exception to indicate what to install ?

This is raised as bug 1016929. If you're interested, please feel free to work on that one too.

Comment 11

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

Comment on attachment 8532798 [details] [diff] [review]
Split test class and name for XUnit formatter

Bouncing this review to James as he's probably the better person to review it. I will say that we don't need to make any changes to the HTML format. We only really need this change so anything processing the xUnit report (such as Jenkins) can understand the package/class structure used.

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8532798 [details] [diff] [review]
Split test class and name for XUnit formatter

Review of attachment 8532798 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but there's a few things to fix up.

::: testing/marionette/client/marionette/marionette_test.py
@@ +541,5 @@
>              time.sleep(0.5)
>          else:
>              raise TimeoutException("wait_for_condition timed out")
>  
> +    def get_test_class_name(self):

It feels like this code should be directly in moztest.adapters.unit

::: testing/mozbase/mozlog/mozlog/structured/formatters/html/html.py
@@ +154,3 @@
>          self.result_rows.append(
>              html.tr([html.td(status_name, class_='col-result'),
> +                     html.td(test_name, class_='col-name'),

I feel like we should always have data['test'] and have the class/method name in addition to this when we know both.

Comment 13

[Julien Pagès (:parkouss)]

Hi James,

I put the code directly in moztest.adapters.unit. The problem is that every javascript test (example tests/unit/test_simpletest_pass.js) now end up like this in the xunit report:

<testcase classname="marionette_test.MarionetteJSTestCase" name="runTest" time="0.43"/>

Because there is no more distinction between python base test class or javascript based test class. I suppose it is a problem, because in results we won't be able to distinguish them (jenkins neither).

Do you have an idea of how I could distinguish them properly in the moztest.adapters.unit module ? An idea is to add some sort of flag to allow a class instance to be handled differently -> if we have some flag, then we fill the extra "classname" and "methodname" to the logger, and the final formatter will use these if they are present.

Maybe something like:

class MarionetteTestCase(CommonTestCase):
    EXTRACT_XUNIT_INFO = True

I can't think at anything best for now - what's your opinion on this ?

Comment 14

[James Graham [:jgraham]]

Yeah, I see the problem. I'm not really sure why this ever worked. 

I believe you still have access to the Testcase instance in the adapter, so you can extend the interface of testcase a little to allow subclasses to implement a getTestMethodName() method as before, and use this, if implemented, to return the method name (i.e. you would use a hasattr(obj, "getTestMethodName") check).

Does that make sense?

Comment 15

[Julien Pagès (:parkouss)]

I think this is what I already implemented in the patch you reviewed:

MarionetteJSTestCase.get_test_class_name
MarionetteJSTestCase.get_test_method_name

(note it is not in MarionetteJSTestCase, to not impact javascript tests)

then it is checked it in the adapter:

if hasattr(test, 'get_test_class_name'):
    extra['class_name'] = test.get_test_class_name()
if hasattr(test, 'get_test_method_name'):
    extra['method_name'] = test.get_test_method_name()

Comment 16

[Julien Pagès (:parkouss)]

Sorry I meant

MarionetteTestCase.get_test_class_name
MarionetteTestCase.get_test_method_name

obviously. Bad copy/paste.

Comment 17

[James Graham [:jgraham]]

Right, but in your original patch the logic to check __class__.__name__ and _testMethodName goes into marionette-specific code, whereas that seems like it should be the default behaviour implemented directly in the adapter. Then the behaviour should be overridden for the js tests where something special is required.

If that still doesn't make sense, lets chat on irc.

Comment 18

[Julien Pagès (:parkouss)]

We talked on irc with James, and now I understand what is required for him.

I now have a question for Dave:

How do you want the javascript marionette tests to be represented in xunit ?

We could put in the 'classname' attribute the full directory (dot separated) to the js file, or maybe just 'JSTest' (or similar) since it seems that all jsfiles are in the same directory.

For the 'methodname', I suppose that just the name of the file (without the directory part) will be good. Maybe without the '.js' extension.

What would you like ?

Also I think I will leave the html formatter unchanged, if that is ok for you. I changed it because it is mentioned in comment 0 but it seems that it is not really helpful.

Comment 19

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

(In reply to Julien Pagès from comment #18)
> We talked on irc with James, and now I understand what is required for him.
> 
> I now have a question for Dave:
> 
> How do you want the javascript marionette tests to be represented in xunit ?
> 
> We could put in the 'classname' attribute the full directory (dot separated)
> to the js file, or maybe just 'JSTest' (or similar) since it seems that all
> jsfiles are in the same directory.
> 
> For the 'methodname', I suppose that just the name of the file (without the
> directory part) will be good. Maybe without the '.js' extension.
> 
> What would you like ?

I don't consume these (and I'm not sure who does) but I think dot separated folders makes sense, and the filename without the extension works for me too for method name.

> Also I think I will leave the html formatter unchanged, if that is ok for
> you. I changed it because it is mentioned in comment 0 but it seems that it
> is not really helpful.

Yeah, makes sense to me.

Comment 20

[Julien Pagès (:parkouss)]

Attachment: Split test class and name for XUnit formatter

I have run this command with success locally:

python runtests.py --binary ../../../../obj-x86_64-unknown-linux-gnu/dist/bin/firefox-bin tests/unit/test_{simpletest_pass.js,click.py} --log-xunit file1.xml

file1.xml:
----------
<?xml version="1.0" encoding="utf8"?>
<testsuite errors="0" failures="0" skips="0" tests="4" time="0.97">
	<testcase classname="home.jp.dev.mozilla.testing.marionette.client.marionette.tests.unit" name="test_simpletest_pass" time="0.32"/>
	<testcase classname="test_click.TestClick" name="test_click" time="0.18"/>
	<testcase classname="test_click.TestClick" name="test_clicking_a_link_made_up_of_numbers_is_handled_correctly" time="0.30"/>
	<testcase classname="test_click.TestClick" name="test_clicking_an_element_that_is_not_displayed_raises" time="0.16"/>
</testsuite>

Comment 21

[James Graham [:jgraham]]

Comment on attachment 8537175 [details] [diff] [review]
Split test class and name for XUnit formatter

Review of attachment 8537175 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks for doing this.

::: testing/marionette/client/marionette/marionette_test.py
@@ +650,5 @@
>          self.marionette.test_name = None
> +
> +    def get_test_class_name(self):
> +        # returns a dot separated folders as class name
> +        dirname = os.path.dirname(self.jsFile).replace('\\', '/')

This is fine, but you could also have just used os.path.sep below.

::: testing/mozbase/moztest/moztest/adapters/unit.py
@@ +106,5 @@
> +        if hasattr(test, 'get_test_class_name'):
> +            class_name = test.get_test_class_name()
> +        else:
> +            class_name = get_test_class_name(test)
> +        if hasattr(test, 'get_test_method_name'):

I wouldn't mind a blank line before this if, and just below before the return.

Comment 22

[Julien Pagès (:parkouss)]

Sure. I had some hesitations for the os.path.sep, but since in the class there is no os.path.realpath or something like that on self.jsFile, I was not sure how it could end up in windows. Sometimes user can pass in unix slash and it works fine on windows too. Anyway, if we are sure here, I can replace it with a split on os.path.sep. :)

I'll fix these soon.

Comment 23

[James Graham [:jgraham]]

Yeah I guess your solution might be more robust against Windows being tolerant of multiple inputs.

Comment 24

[Julien Pagès (:parkouss)]

Attachment: Split test class and name for XUnit formatter

Ok then, I have just added the blank lines.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1822066cfe20

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1822066cfe20