Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#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: jessica.mckellar@…, thijs Branch: branches/curlarg-eliminate-4162
(diff, github, buildbot, log)
Author: PenguinOfDoom Launchpad Bug:

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 5 years ago by spiv

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.

comment:2 Changed 4 years ago by glyph

  • Owner changed from glyph to PenguinOfDoom

comment:3 Changed 4 years ago by PenguinOfDoom

_c_urlarg is used by mail as well as web, so core is the place for it.

comment:4 Changed 4 years ago by pahan

  • Author set to pahan
  • Branch set to branches/curlarg-tests-4162

(In [28754]) Branching to 'curlarg-tests-4162'

comment:5 Changed 4 years ago by PenguinOfDoom

  • Keywords review added
  • Owner PenguinOfDoom deleted

comment:6 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to PenguinOfDoom

comment:7 Changed 4 years ago by PenguinOfDoom

  • Keywords review added
  • Owner PenguinOfDoom deleted

Ha ha look how dumb I am. I committed some changes to the branch now.

comment:8 Changed 4 years ago by PenguinOfDoom

  • Priority changed from normal to highest

comment:9 Changed 4 years ago by jesstess

  • Owner set to jesstess

comment:10 Changed 4 years ago by jesstess

  • Cc jessica.mckellar@… 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 4 years ago by thijs

  • Cc thijs added
  • Owner changed from PenguinOfDoom to cyli

let me know if the branch needs to be merged forward.

comment:12 Changed 4 years ago by cyli

  • Owner changed from cyli 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 3 years ago by <automation>

  • Owner PenguinOfDoom deleted

comment:14 Changed 3 years ago by pahan

  • Branch changed from branches/curlarg-tests-4162 to branches/curlarg-eliminate-4162

(In [31154]) Branching to 'curlarg-eliminate-4162'

comment:15 Changed 3 years ago by PenguinOfDoom

  • Author changed from pahan to 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 3 years ago by lvh

  • Owner set to lvh
  • Status changed from new to assigned

comment:17 Changed 3 years ago by lvh

  • Owner changed from lvh to PenguinOfDoom
  • Status changed from assigned to new

comment:18 Changed 3 years ago by lvh

  • 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:19 Changed 3 years ago by PenguinOfDoom

  • Keywords review added
  • Owner PenguinOfDoom deleted

Oops. Fixed!

comment:20 Changed 3 years ago by lvh

  • Keywords review removed

Looks good! Merge ftw

comment:21 Changed 3 years ago by pahan

  • Resolution set to fixed
  • Status changed from new to closed

(In [31167]) Merge curlarg-eliminate-4162: Remove _c_urlarg

Author: PenguinOfDoom
Reviewer: lvh
Fixes: #4162

comment:22 Changed 3 years ago by PenguinOfDoom

#4246 is a duplicate of this

Note: See TracTickets for help on using tickets.