Opened 5 years ago

Last modified 4 years ago

#6115 defect new

twisted.web.http.parseContentRange has no unit tests

Reported by: Jean-Paul Calderone Owned by: moijes12
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, moijes12 Branch:
Author:

Description

It should be tested (found while porting http.py to Python 3).

It's only used by twisted.web.client, and presumably there is no test coverage for the code there that is using it.

Attachments (1)

6115.patch (2.3 KB) - added by moijes12 4 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: jknight added

Changed 4 years ago by moijes12

Attachment: 6115.patch added

comment:2 Changed 4 years ago by moijes12

Cc: moijes12 added
Keywords: review added

Hi

I have attached a patch with tests for the twisted.web.http.parseContentRange function. The bin/trial program has been run and no new issues have been added. Below are some points that may help anyone reviewing the code:

  1. DummyRequest has been imported from test_web for creation of dummy requests containing the range header. A good number of tests for content headers are in the twisted.web.http.test.test_static file and I used them as a reference. Would it be better to use a http.Request
  1. Looking at the code, I could come up with 3 scenarios for testing:
    1. parseContentRange raises a ValueError if the content-range header does not specify the range in bytes.
    2. parseContentRange returns realLength as None if it was set as *.
    3. parseContentRange returns start, end and realLength properly.

comment:3 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to moijes12
  1. There doesn't appear to be any reason to be creating a DummyRequest here. parseContentRange just takes a string.
  2. Inspecting the code, the error message for non-bytes is broken (it has a % subsitution, but that subsitution doesn't get made. This should be tested and fixed.
    • Incidentally, that code path doesn't appear covered, since test_raiseValueErrorForNonByteData will actually fail on assigning the result of the split.
  3. There should perhaps be test for various other invalid data (non-numeric data, or more fields.

comment:4 Changed 4 years ago by Tom Prince

coverage run --branch bin/trial twisted.web.test.test_http
coverage html -i twisted/web/http.py
firefox htmlcov/twisted_web_http.html

Also, TestCase docstrings typically include a L{}ink to the functions/method/classes under test.

Note: See TracTickets for help on using tickets.