Opened 4 years ago

Last modified 4 years ago

#6116 defect new

twisted.web.http.stringToDatetime has incomplete test coverage

Reported by: Jean-Paul Calderone Owned by: Can Ibanoglu
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/web-stringToDatetime-6116
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

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

Attachments (1)

ticket6116.patch (1.5 KB) - added by Can Ibanoglu 4 years ago.
Added tests for twisted.web.http.stringToDatetime

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 4 years ago by Can Ibanoglu

Owner: set to Can Ibanoglu

comment:3 Changed 4 years ago by Can Ibanoglu

I have written three tests for http.stringToDatetime but I'm a bit unsure about how to write the newsfile. I was thinking something along the lines of:

twisted.web.http.stringToDatetime now has complete test coverage

Another question I have is directly related to the tests themselves. The only current test for this method tests the roundtrip 10000 times but the tests I've written test their respective parts just once. Should I modify the tests so that they work in a similar fashion?

Also, even if I've written a test case specifically for year<100, the coverage report still says that it has partial coverage. Any help related to that is very welcome.

comment:4 Changed 4 years ago by Thijs Triemstra

hi canibanoglu, i cant answer your question but by attaching a patch with the tests you've written it'll probably be easier to help, and then bringing up this ticket on the twisted irc channel..

comment:5 Changed 4 years ago by Thijs Triemstra

If the ticket doesn't change anything to the code but only the tests or docstrings we don't bother to describe the change and simply use an empty .misc file.

Changed 4 years ago by Can Ibanoglu

Attachment: ticket6116.patch added

Added tests for twisted.web.http.stringToDatetime

comment:6 Changed 4 years ago by Can Ibanoglu

Keywords: review added
Owner: Can Ibanoglu deleted

comment:7 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/web-stringToDatetime-6116

(In [38684]) Branching to web-stringToDatetime-6116.

comment:8 Changed 4 years ago by Tom Prince

(In [38685]) Apply ticket6116.patch from canibanoglu.

Refs: #6116

comment:9 Changed 4 years ago by Tom Prince

Owner: set to Can Ibanoglu

Thanks for your contribution. A couple of points:

  1. While running coverage.py shows that with your tests stringToDateTime is comepletly covered, there are still some code paths that aren't excercised. Namely, the if statements that have multiple possible ways of succeeding. I noticed the following case, but further consideration might reveal some more:
    1. The attempt to add 'Sun,' to the begining is only exercised in the case where the format is invalid and never in a case that adding the date leads to a succeful conversion.
    2. None of the test exercise the code that supports dropping 'GMT'.
    3. The fallthrough case of the second format isn't tested.
    4. It would be nice if there where some explicit tests of the first format, rather than just the round-trip test. (Also for dateTimeToString).
  2. (minor) Test docstrings should have "Test that" or "Test wether". Rather, they should be something like "When <this is done>, <that happens>. (See #6301)
  3. (minor) There is no inherent ordering of the formats, so the tests should have some more descriptive names. The RFC suggests calling them "RFC 850 format" and "asctime() format". (And IMF-fixdate for the first one).

Please resubmit for review after addressing the above points.

comment:10 Changed 4 years ago by Tom Prince

Keywords: review removed

comment:11 Changed 4 years ago by Can Ibanoglu

Thanks a lot for the review! I will address your points as soon as I'm done with school stuff and resubmit. Thanks again :)

comment:12 Changed 4 years ago by Can Ibanoglu

OK, back to work.

I have renamed the test methods so that they now mention the names kindly provided by tom.prince (RFC 850, etc.) I have fixed the docstrings for the methods. I have added cases where the weekday part of the datetime string is omitted (in the hopes of addressing 1.1 from tom.prince's review) I have added cases to two of the datetime formats which drop the trailing "GMT". I haven't done this with asctime() format.

I just have two questions before I submit a patch.

1) Apart from removing the trailing "GMT" and removing the weekday part of the datetime string from the first datetime format (IMF-fixdate format), I couldn't think of testing anything else with this format. Am I missing something?

2) The

elif year < 100:

line always gets partial coverage and I'm afraid I don't understand why this happens. I had written a case which had a year greater than 69 and less than 100 and the test still passes but coverage still reports partial coverage on that line. I would really appreciate a pointer for this part.

Thanks a lot in advance for the help.

Note: See TracTickets for help on using tickets.