Opened 5 years ago

Last modified 4 years ago

#6160 task new

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

Reported by: Itamar Turner-Trauring Owned by: Thijs Triemstra
Priority: normal Milestone:
Component: core Keywords:
Cc: Thijs Triemstra Branch: branches/nametolabel-tests-6160
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs

Description

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

Attachments (3)

twisted-python-test-test_utilpy3.patch (1.4 KB) - added by Drew Dudash 5 years ago.
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.
twisted-python-test-test_utilpy3.2.patch (4.2 KB) - added by Drew Dudash 5 years ago.
Newer update version, handles indentifiers with numbers and underscores properly.
twisted-python-test-test_utilpy3.3.patch (839 bytes) - added by Drew Dudash 5 years ago.
Alters Unit Tests Only

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by Drew Dudash

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.

comment:1 Changed 5 years ago by Drew Dudash

Type: defecttask

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.

Changed 5 years ago by Drew Dudash

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

comment:2 Changed 5 years ago by Jean-Paul Calderone

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

Changed 5 years ago by Drew Dudash

Alters Unit Tests Only

comment:3 Changed 4 years ago by Drew Dudash

Keywords: review added; easy removed

comment:4 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

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.

comment:5 Changed 4 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/nametolabel-tests-6160

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

comment:6 Changed 4 years ago by Jean-Paul Calderone

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

refs #6160

comment:7 Changed 4 years ago by Jean-Paul Calderone

Author: exarkunPetit_Dejeuner
Owner: changed from Jean-Paul Calderone to Drew Dudash
Status: assignednew

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

comment:8 Changed 4 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:9 Changed 4 years ago by Thijs Triemstra

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

comment:10 Changed 4 years ago by Thijs Triemstra

Author: Petit_Dejeunerthijs
Keywords: review added
Owner: Drew Dudash deleted

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

comment:11 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: set to Thijs Triemstra

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.

Note: See TracTickets for help on using tickets.