twisted.python.util.nameToLabel is missing some test coverage

[itamar]

twisted.python.util.nameToLabel only has minimal tests that don't cover all cases. New tests should be written to provide full coverage.

[Petit_Dejeuner]

Alters both twisted.python._utilpy3 and twisted.python.test.test_utilpy3. Fixes an extra whitespace bug in the former and adds more test cases to the latter.

[Petit_Dejeuner]

I've added some more tests, but as of now the function doesn't really support anything but camel or pascal case. I'll work on rewriting it to accept identifiers with numbers and underscores, and maybe raise an error when given an invalid identifier.

It also has some issues when all caps is mixed with lowercase. You would assume the following. "FOObar" -> "FOO Bar" But instead you get "FOObar" -> "FO Obar"

I went ahead and altered the return line with a call to'.split()' to make sure that no white space appeared in the front of the text. It's a current bug that happens when using all caps.

[Petit_Dejeuner]

Newer update version, handles indentifiers with numbers and underscores properly.

[exarkun]

I've added some more tests, but as of now the function doesn't really support anything but camel or pascal case. I'll work on rewriting it to accept identifiers with numbers and underscores, and maybe raise an error when given an invalid identifier.

Thanks for your work on this. Please file a new ticket for other improvements to this function and leave this ticket only for adding unit tests for the existing functionality. The function can't be changed (bugs fixed, new features added) until it has full test coverage.

Also, in case you intended to submit this patch for review, make sure you're familiar with the ReviewProcess (in particular, you may have missed adding the "review" keyword to this ticket).

[Petit_Dejeuner]

Alters Unit Tests Only

[exarkun]

Thanks. There are still a couple issues:

1. One of the new cases fails, nameToLabel('F') == ' F' != 'F'
2. There are still uncovered lines in nameToLabel. The goal of this ticket should be to have full branch and line coverage of the nameToLabel implementation.

[exarkun]

(In [36813]) Branching to 'nametolabel-tests-6160'

[exarkun]

(In [36814]) Apply twisted-python-test-test_utilpy3.3.patch

refs #6160

[exarkun]

I updated the patch to reflect that _utilpy3.py is gone now and checked the result into the linked branch.

[thijs]

(In [38153]) add test coverage and news file. refs #6160

[thijs]

Added full coverage and forced a  build. Up for review.

[rwall]

Thanks Thijs,

I ran coverage and I can see that nameToLabel does now have complete coverage.

coverage run --source=twisted.python.util ./bin/trial twisted.python.test.test_util

A few points:

1. I see that you replaced the tests introduced in the original patch (r38153) with a single test that achieves the same coverage, but I think I preferred the original tests. They made it clear what the behaviour of function should be, regardless of it's current implementation.
2. The new "('aCRoNym', 'A C Ro Nym')" test is difficult to read. It's not obvious what the words *should* be. I'd prefer if the test used "foo bar baz" like the other tests. I think it would be even better if the tests used realistic example words. eg HTTP Request Headers.
3. I think there should be a test demonstrating the behaviour when the first word is an acronym. eg HTTPRequest which leads to a leading space - a bug which needs fixing in a future ticket.

       Index: twisted/python/test/test_util.py
       ===================================================================
       --- twisted/python/test/test_util.py	(revision 38407)
       +++ twisted/python/test/test_util.py	(working copy)
       @@ -109,6 +109,8 @@
                    ('fooBarBaz', 'Foo Bar Baz'),
                    ('aCRoNym', 'A C Ro Nym'),
                    ('FF', ' FF'),
       +            ('HTTPRequest', ' HTTP Request'),
       +            ('originalHTTPRequest', 'Original HTTP Request'),
                ]
                for inp, out in nameData:
                    got = util.nameToLabel(inp)
   

4. Also petit_dejeuner still needs to raise and link to ticket(s) for the other suggested improvements and bug fixes in comment:2.

Please answer or address the above points and resubmit for review.

-RichardW.