#4162 defect closed fixed (fixed)
_c_urlarg.c tests should be in the core, not in web
Reported by: | spiv | Owned by: | |
---|---|---|---|
Priority: | highest | Milestone: | |
Component: | core | Keywords: | |
Cc: | jesstess, Thijs Triemstra | Branch: |
branches/curlarg-eliminate-4162
branch-diff, diff-cov, branch-cov, buildbot |
Author: | PenguinOfDoom |
Description
_c_urlarg.c
is part of the core (it is in twisted/protocols
), but its unit tests are in twisted/web/test/test_http.py
. Those tests should be in the core somewhere instead.
This was originally mentioned in #4142.
Change History (22)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
Owner: | changed from Glyph to PenguinOfDoom |
---|
comment:3 Changed 8 years ago by
_c_urlarg is used by mail as well as web, so core is the place for it.
comment:4 Changed 8 years ago by
Author: | → pahan |
---|---|
Branch: | → branches/curlarg-tests-4162 |
(In [28754]) Branching to 'curlarg-tests-4162'
comment:5 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | PenguinOfDoom deleted |
comment:6 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to PenguinOfDoom |
You forgot to have some revisions: http://twistedmatrix.com/trac/log/branches/curlarg-tests-4162
comment:7 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | PenguinOfDoom deleted |
Ha ha look how dumb I am. I committed some changes to the branch now.
comment:8 Changed 8 years ago by
Priority: | normal → highest |
---|
comment:9 Changed 8 years ago by
Owner: | set to jesstess |
---|
comment:10 Changed 8 years ago by
Cc: | jesstess added |
---|---|
Keywords: | review removed |
Owner: | changed from jesstess to PenguinOfDoom |
Thanks for working on this, PenguinOfDoom. Some feedback:
- I don't know if being a C file changes how this works, but presumably _c_urlarg.py needs something like /* -*- test-case-name: twisted.protocol.test.test_c_urlarg -*- */ at the top. If that gets added, the file needs a copyright bump.
- twisted/web/test/test_http.py has pyflakes warnings about the unused urllib import and the unused transport on line 1391.
- test_http.py has one spot with trailing whitespace.
- the tests in test_c_urlarg.py need have names of the form test_myTest, and should be using assert forms instead of fail forms as per the test standard. The docstrings on the tests are vague and aren't actually describing the behavior we should be seeing, and testUnquote is testing multiple behaviors, which should really be split into their own tests. Can you fix these things here or open a new ticket for them?
comment:11 Changed 8 years ago by
Cc: | Thijs Triemstra added |
---|---|
Owner: | changed from PenguinOfDoom to Ying Li |
let me know if the branch needs to be merged forward.
comment:12 Changed 8 years ago by
Owner: | changed from Ying Li to PenguinOfDoom |
---|
This looks like it's waiting for the author (PenguinOfDoom) to respond to review feedback - not sure why it was assigned to me.
comment:13 Changed 7 years ago by
Owner: | PenguinOfDoom deleted |
---|
comment:14 Changed 7 years ago by
Branch: | branches/curlarg-tests-4162 → branches/curlarg-eliminate-4162 |
---|
(In [31154]) Branching to 'curlarg-eliminate-4162'
comment:15 Changed 7 years ago by
Author: | pahan → PenguinOfDoom |
---|---|
Keywords: | review added |
There is no credible evidence that _c_urlarg ever sped anything up, and it certainly has caused its share of problems over the years. Here's a patch to delete it!
comment:16 Changed 7 years ago by
Owner: | set to lvh |
---|---|
Status: | new → assigned |
comment:17 Changed 7 years ago by
Owner: | changed from lvh to PenguinOfDoom |
---|---|
Status: | assigned → new |
comment:18 Changed 7 years ago by
Keywords: | review removed |
---|
Apparently there is a failing test, but I don't see how it's related...
http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/curlarg-eliminate-4162
comment:21 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
As idnar commented on IRC, another option might be to move _c_urlarg itself to web. I'm don't have a strong opinion either way.