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

Proposed by Martin Packman
Status: Merged
Merged at revision: 20
Proposed branch: lp://qastaging/~gz/pyjunitxml/split_test_id_before_parameter
Merge into: lp://qastaging/pyjunitxml
Prerequisite: lp://qastaging/~gz/pyjunitxml/unexpected_expectations_python_2.7
Diff against target: 46 lines (+18/-6)
2 files modified
junitxml/__init__.py (+6/-6)
junitxml/tests/test_junitxml.py (+12/-0)
To merge this branch: bzr merge lp://qastaging/~gz/pyjunitxml/split_test_id_before_parameter
Reviewer Review Type Date Requested Status
Vincent Ladeuil Needs Fixing
Robert Collins Pending
Review via email: mp+35138@code.qastaging.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

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
Revision history for this message
Martin Packman (gz) wrote :

> 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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> 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

Revision history for this message
Martin Packman (gz) wrote :

> 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. :)

Revision history for this message
Robert Collins (lifeless) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

Needs NEWS.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: