Opened 2 years ago

Last modified 19 months ago

#6160 task new

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

Reported by: itamar Owned by: thijs
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/nametolabel-tests-6160
(diff, github, buildbot, log)
Author: thijs Launchpad Bug:

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

Download all attachments as: .zip

Change History (14)

Changed 2 years ago by 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.

comment:1 Changed 2 years ago by Petit_Dejeuner

  • Type changed from defect to task

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 2 years ago by Petit_Dejeuner

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

comment:2 Changed 2 years ago by 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).

Changed 2 years ago by Petit_Dejeuner

Alters Unit Tests Only

comment:3 Changed 2 years ago by Petit_Dejeuner

  • Keywords review added; easy removed

comment:4 Changed 23 months ago by exarkun

  • Keywords review removed
  • Owner set to exarkun
  • Status changed from new to assigned

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 23 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/nametolabel-tests-6160

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

comment:6 Changed 23 months ago by exarkun

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

refs #6160

comment:7 Changed 23 months ago by exarkun

  • Author changed from exarkun to Petit_Dejeuner
  • Owner changed from exarkun to Petit_Dejeuner
  • Status changed from assigned to new

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

comment:8 Changed 22 months ago by thijs

  • Cc thijs added

comment:9 Changed 19 months ago by thijs

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

comment:10 Changed 19 months ago by thijs

  • Author changed from Petit_Dejeuner to thijs
  • Keywords review added
  • Owner Petit_Dejeuner deleted

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

comment:11 Changed 19 months ago by rwall

  • Keywords review removed
  • Owner set to thijs

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.