Merge lp:~gz/pyjunitxml/split_test_id_before_parameter into lp:pyjunitxml

[Martin Packman]

Babune has a little weirdness in its results like:
<http://babune.ladeuil.net:24842/job/selftest-maverick/42/testReport/bzrlib.tests.test_upgrade_stacked.TestStackUpgrade.test_stack_upgrade(1.6,%201/>

Branch contains a simple change to the way test ids are split to stop treating dots inside parameters as a good point to split the test class from the test name.

[Vincent Ladeuil (vila)]

Hmm, what are the constraints on a test id ?

I've got the feeling that the (par1,par2) addition is quite bzr specific
and I don't know if we even enforce the constraints that parameter strings should not embed
any funny stuff (say a '(') which will break your fix as well.

14 + class_end = test_id.rfind(".", 0, test_id.find("("))

So I'd be very tempted to ask for a better way than test_id.find("(") to identify
a valid python identifier at the *start* of the test id and then, and only then,
extract the class from that.

review: Needs Fixing

[Martin Packman (gz)]

> Hmm, what are the constraints on a test id ?

Effectively none, but in practice tests will normally be identifiers joined by dots:
<http://docs.python.org/reference/lexical_analysis.html#identifiers>

> I've got the feeling that the (par1,par2) addition is quite bzr specific
> and I don't know if we even enforce the constraints that parameter strings
> should not embed
> any funny stuff (say a '(') which will break your fix as well.

Actually this way shouldn't care about anything in the parameter as it's searching for the first opening bracket and never looking beyond it. I agree this is probably somewhat bzr specific, but should at least be harmless on less fancy setups.

> 14 + class_end = test_id.rfind(".", 0, test_id.find("("))
>
> So I'd be very tempted to ask for a better way than test_id.find("(") to
> identify
> a valid python identifier at the *start* of the test id and then, and only
> then,
> extract the class from that.

Well, [^._0-9a-z-A-Z] would be an option, but is probably overkill and *more* likely to break on other suite's fancy id schemes.

[Vincent Ladeuil (vila)]

>>>>> Martin [gz] <email address hidden> writes:

    >> Hmm, what are the constraints on a test id ?
    > Effectively none, but in practice tests will normally be
    > identifiers joined by dots:
    > <http://docs.python.org/reference/lexical_analysis.html#identifiers>

    >> I've got the feeling that the (par1,par2) addition is quite bzr
    > specific
    >> and I don't know if we even enforce the constraints that
    > parameter strings
    >> should not embed
    >> any funny stuff (say a '(') which will break your fix as well.

    > Actually this way shouldn't care about anything in the parameter
    > as it's searching for the first opening bracket and never looking
    > beyond it. I agree this is probably somewhat bzr specific, but
    > should at least be harmless on less fancy setups.

Meh, of course, why did I read this find as a rfind...

    >> 14 + class_end = test_id.rfind(".", 0, test_id.find("("))
    >>
    >> So I'd be very tempted to ask for a better way than
    > test_id.find("(") to
    >> identify
    >> a valid python identifier at the *start* of the test id and
    > then, and only
    >> then,
    >> extract the class from that.will

    > Well, [^._0-9a-z-A-Z] would be an option, but is probably
    > overkill and *more* likely to break on other suite's fancy id
    > schemes.

Meh for you this time :-p Why would it break ?

It will instead works for [] "" '' {} in addition to '()'. Any descent
scanner (which can do an indirection through a table of valid chars vs a
char comparison) shouldn't make such a difference in performance
compared with the range copyings that will follow.

Bah, maybe I've used perl too much:

  ($class, $test) = $id =~ /^([.\w]+)\.(\w+.*$)/

No useless variables, no if, no split, no slice, no nuthin :)

P.S.: No religious war intended, I love python and use emacs :-P

[Martin Packman (gz)]

> Meh, of course, why did I read this find as a rfind...

Because that line isn't very easy to read, or obvious in what it's doing. Splitting it up doesn't really help though, so I'm relying on people trusting the comment.

> Meh for you this time :-p Why would it break ?

Well, why should another fancy id scheme not use some non-word characters as a prefix rather than suffix?

> Bah, maybe I've used perl too much:
>
> ($class, $test) = $id =~ /^([.\w]+)\.(\w+.*$)/
>
> No useless variables, no if, no split, no slice, no nuthin :)

Python just isn't very good at string-wrangling, I don't think that's controversial. Not having to think for five minutes about what every line of code actually means does have benefits though. :)

[Robert Collins (lifeless)]

That is a little weird indeed.

For future ref, the subunit id() style is what is largely in mind here, but yes, dotted names is likely in most languages, so its a good heuristic.

[Robert Collins (lifeless)]

Needs NEWS.

=== modified file 'junitxml/__init__.py'
--- junitxml/__init__.py	2010-09-10 17:07:43 +0000
+++ junitxml/__init__.py	2010-09-10 17:07:43 +0000
@@ -96,13 +96,13 @@
 
     def _test_case_string(self, test):
         duration = self._duration(self._test_start)
-        prefix_suffix = test.id().rsplit('.', 1)
-        if len(prefix_suffix) == 1:
-            classname = ""
-            name = prefix_suffix[0]
+        test_id = test.id()
+        # Split on the last dot not inside a parameter
+        class_end = test_id.rfind(".", 0, test_id.find("("))
+        if class_end == -1:
+            classname, name = "", test_id
         else:
-            classname = prefix_suffix[0]
-            name = prefix_suffix[1]
+            classname, name = test_id[:class_end], test_id[class_end+1:]
         self._results.append('<testcase classname="%s" name="%s" '
             'time="%0.3f"' % (escape(classname), escape(name), duration))
 

=== modified file 'junitxml/tests/test_junitxml.py'
--- junitxml/tests/test_junitxml.py	2010-09-10 17:07:43 +0000
+++ junitxml/tests/test_junitxml.py	2010-09-10 17:07:43 +0000
@@ -87,6 +87,18 @@
         output = self.get_output()
         self.assertTrue('tests="2"' in output)
 
+    def test_test_id_with_parameter(self):
+        class Passes(unittest.TestCase):
+            def id(self):
+                return unittest.TestCase.id(self) + '(version_1.6)'
+            def test_me(self):
+                pass
+        self.result.startTestRun()
+        Passes("test_me").run(self.result)
+        self.result.stopTestRun()
+        output = self.get_output()
+        self.assertTrue('Passes" name="test_me(version_1.6)"' in output)
+
     def test_erroring_test(self):
         class Errors(unittest.TestCase):
             def test_me(self):