Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1762 (closed enhancement: fixed)

Opened 10 years ago

Last modified 9 years ago

[PATCH] Use WebTest for unit tests

Reported by: lmacken Owned by: kskuhlman
Priority: high Milestone: 1.1
Component: TurboGears Version: 1.1 HEAD
Severity: critical Keywords:
Cc: mramm, faide, ctrochalakis

Description

We need the ability to easily use WebTest? for unit testing. This will allow us to directly test with WebOb? Request/Response? objects.

Attachments

TurboGears-1.0-WebTest.patch Download (2.6 KB) - added by lmacken 10 years ago.
TurboGears-1.1-WebTest.patch Download (83.8 KB) - added by lmacken 10 years ago.
Updated again with some minor cleanups
webtest-ken.patch Download (134.9 KB) - added by kskuhlman 9 years ago.
Ken's attempt
webtest-luke-tweaked.patch Download (85.9 KB) - added by kskuhlman 9 years ago.
Minor tweaks to fix regressions in Luke's version
merge_rpt.txt Download (3.5 KB) - added by kskuhlman 9 years ago.
A report of which changes were kept from the two approaches.
webtest_blended.patch Download (141.7 KB) - added by kskuhlman 9 years ago.
Patch including a blending of Luke & Ken's approaches.
tgtest.patch Download (11.0 KB) - added by kskuhlman 9 years ago.
Almost complete..

Change History

comment:1 Changed 10 years ago by lmacken

  • Status changed from new to assigned
  • Cc mramm added

Here is my first attempt at adding WebTest unit testing support to TG1.0.

This patch currently leaves testutil.{call,create_request} alone, and simply adds new TGWebTest and DBWebTest classes. Whether or not we want to port create_request/call to use WebTest, or deprecate them, is still up for discussion. I just want to touch base with my current implementation before I dig deeper into it and start porting it up to 1.1 and 2.0.

The TGWebTest class fires automatically sets up turbogears and hooks up WebTest to the CherryPy WSGI server. 'self.app' can then be used by the unit tester to interact with WebTest. The DBWebTest class does the same thing, while also setting up and tearing down the database as well.

My patch also adds SQLAlchemy support to the testutil.DBTest class. It seems to work fine in all of my testing, but could definitely use some more eyes on it :)

The attach patch allows for the following types of unit tests:

class TestPages(testutil.DBWebTest):

    def test_forbidden(self):
        # Hot action is forbidden
        res = self.app.get('/hot_action', status=403)

    def test_webob_response(self):
        user = User(user_name=u"test", password=u"test")
        self.login_user(user)
        res = self.app.get('/hot_action')
        assert "Hot WSGI action" in res

    def test_response_namespace(self):
        user = User(user_name=u"test", password=u"test")
        self.login_user(user)
        res = self.app.get('/hot_action')
        assert res.namespace['tg_flash'] == u'Hot WSGI action'

Questions, Comments, Suggestions ?

Changed 10 years ago by lmacken

comment:2 Changed 10 years ago by lmacken

  • Summary changed from Use WebTest for unit tests to [PATCH] Use WebTest for unit tests

I pulled the testutil.DBTest SQLAlchemy integration out into it's own patch and added it to ticket 1764.

comment:3 Changed 10 years ago by mramm

  • Milestone changed from 2.0 to 1.1

comment:4 Changed 10 years ago by lmacken

Ok, so the above patch is almost complete, but still has some issues, the main problem being:

  • There are a few places, mainly in the identity provider, where we need to deal with things like cherrypy.request.identityProvider. Trying to munge this ourselves to work around it yields AttributeError?: cherrypy.request has no properties outside of an HTTP request..

Any suggestions?

Changed 10 years ago by lmacken

Updated again with some minor cleanups

comment:5 Changed 10 years ago by lmacken

Here is another issue,

======================================================================
FAIL: flash redirect with chars that can cause troubles in cookies
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/lmacken/tg1.1/turbogears/turbogears/tests/test_controllers.py", line 475, in test_flash_redirect_with_trouble_chars
    assert u'k\xe4se'.encode('utf-8') in value
AssertionError

The orignal test case yields something like this

cherrypy.response.simple_cookie["tg_flash"].value = '%24foo%2C%20k\xc3\xa4se%3B%09bar!'

When porting it to WebTest? and dealing with a WebOb? response, I'm getting this:

response.cookies_set['tg_flash'] = '"%24foo%2C%20k\\303\\244se%3B%09bar!"'

comment:6 Changed 9 years ago by kskuhlman

  • Cc kskuhlman added

A few things:

  • I'm working on cleaning up the patch I submitted against #1466, which does essentially the same things as Luke's patch here. I'm almost done & will post an update to this ticket once I've done a final sanity check, compared it more fully to Luke's approach, & written a test migration guide.
  • There were some test isolation issues where one test module was relying on a request being created as a side-effect of another test module. Since requests are (properly) short-lived in a webtest world, these dependencies needed to be fixed prior to the migration. These were fixed in r4584, r4595, and r4596.
  • If we end up going with Luke's patch, test_catwalk needs another look. The svn:executable property was set on that module when Luke did his work; since nose skips executables by default, these tests weren't getting run & have some problems yet. (I removed the executable bit in r4583).
  • It looks like Luke's latest patch fixes the identity issues he mentioned above. As far as the cookie encoding issue, I think that test is overly specific about how the encoding should happen & is too fragile. The important thing to test is that the "trouble chars" (ie, unicode) can be encoded & decoded & returns the proper result, not that the intermediate result is a particular value. You could probably get it to work using util.quote_cookie & unquote_cookie, but you'd still have a fragile test that has a lot of extra steps without testing anything new.
  • We should formally deprecate create_request & createRequest. I added a decorator for this purpose in r4306 (& fixed in r4585).

Anyway, I haven't reviewed Luke's patch too closely yet, but it looks pretty good at first glance in as far as it goes. I think we should add some scope, though, to get rid of other dependencies on CP2 internals. I'm not going to go into detail on that here, since it overlaps what I need to do for my first point above.

Changed 9 years ago by kskuhlman

Ken's attempt

comment:7 Changed 9 years ago by kskuhlman

Attached my attempt at this ticket, and created a "Work In Progress" page at  http://docs.turbogears.org/1.1/TestMigration

I'm going to review Luke's version more closely now & see if I can fix the handful of regressions I noticed when testing with it earlier. After that, I'll look into whether a hybrid approach is possible/useful (it should be.. I already have a short list of things I like/don't like from each approach).

Changed 9 years ago by kskuhlman

Minor tweaks to fix regressions in Luke's version

comment:8 Changed 9 years ago by kskuhlman

That was easy. Basically just changed response to response.body for the json loads in test_catwalk, moved the remaining request-based tests in test_identity inside the controllers, and made the cookie-encoding test change I suggested earlier.

"webtest-luke-tweaked.patch" is just Luke's patch with these adjustments.

comment:9 Changed 9 years ago by kskuhlman

  • Version changed from trunk to 1.1 HEAD
  • Milestone changed from 1.1 to 1.1.1

I'm very happy with the way the merger turned out. The result is cleaner than my original version & more complete than Luke's.

I've updated the TestMigration page that I created earlier, and am about to attach a new patch & a merger report.

There's one minor wart: in widgets/tests/test_request_related_features.py, there's one test that requires the server to be stopped & restarted. The problem is the validate decorator's mangling of parameters is buggy. I think this has just been hidden by all the starting & stopping of the server that's been happening in the tests, so I've just marked this with a FIXME.

If no-one objects, I'm going to commit this in the next couple of days, after I review the changes one more time & do a "final" cleanup of the docs.

Changed 9 years ago by kskuhlman

A report of which changes were kept from the two approaches.

Changed 9 years ago by kskuhlman

Patch including a blending of Luke & Ken's approaches.

comment:10 Changed 9 years ago by lmacken

Nice work, Ken! I'm very pleased with the results as well. The patch looks to be like best of both worlds, gives us a clean consistent API, and shaves about 32 seconds off of the TurboGears test suite. Looks good to me :)

comment:11 Changed 9 years ago by kskuhlman

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

Applied in r4694 & r4695. Thanks for all the work, Luke!

comment:13 Changed 9 years ago by kskuhlman

  • Milestone changed from 1.6 to 1.5

comment:14 Changed 9 years ago by kskuhlman

  • Priority changed from normal to high
  • Status changed from closed to reopened
  • Resolution fixed deleted
  • Severity changed from normal to critical
  • Milestone changed from 1.5 to 1.1

When I merged Luke & my approaches, I failed to update the quickstart templates, so the tests for newly created projects are erroring.

Also, chrisz has suggested that TGWebTest have a root attribute, which setUp would use. That way in the common case it wouldn't be necessary to override setUp & tearDown. I'll fix that while I'm at it.

Finally, I plan on renaming TGWebTest to just TGTest, since the shorter name is more memorable.

Re-opening ticket and raising priority.

comment:15 Changed 9 years ago by kskuhlman

  • Status changed from reopened to new
  • Owner changed from lmacken to kskuhlman

Changed 9 years ago by kskuhlman

Almost complete..

comment:16 Changed 9 years ago by kskuhlman

  • Cc faide added; kskuhlman removed
  • Status changed from new to assigned

The patch I just attached is almost complete. Identity-enabled quickstarted apps using SQLObject pass tests, but SQLAlchemy & Elixir apps error with "Class '<class 'turbogears.visit.savisit.TG_Visit'>' already has a primary mapper defined"

I'll look into the remaining issues later tonight. We need this fixed by 1.1beta-1.

comment:17 Changed 9 years ago by kskuhlman

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

Confirmed that the SA mapper problem existed before this patch, so going ahead and committed it in r5276 & closing this ticket. Created a new ticket, #1969, for the remaining problem.

comment:18 Changed 9 years ago by ctrochalakis

  • Cc ctrochalakis added
Note: See TracTickets for help on using tickets.