Extensions Docs Server: Integration testing

Extensions Docs Server: Integration testing

A test that makes sure all the pages in the templates/public directory render
without erroring.

BUG=131095
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149101

[cduvall, 2012-07-26 23:46:33]

Testing yay!

[not at google - send to devlin, 2012-07-30 16:37:37]

That's really cool. Nice approach.

What would be *even better* is if we could exercise the handler code directly,
but to do that presumably we need an appengine instance running and stuff.
Something like:

import echo_handler

class IntegrationTest(unittest.TestCase):
  handler = echo_handler.Handler()
  handler.get('/chrome/extensions/storage.html')

Presumably it wouldn't look anything like that because you'd need to instantiate
a handler per public template, but yeah. You might need the equivalent of
start_dev_server for that, not sure.

Oh, and if/when you do, don't forget to exercise the warmup request.

---

Either way we should consider submitting this unit test anyway, because I can
see it being valuable.

[cduvall, 2012-07-30 21:00:20]

This works way better, I moved all the logic into server_instance, and now I can
use GetInstanceForBranch directly.

I wasn't sure how to handle the warmup requests since they are handled in
echo_handler, and that imports things from Appengine. We can't expect everyone
who runs these tests to download Appengine, so I'm still thinking about how to
handle that.

[not at google - send to devlin, 2012-07-30 21:19:09]

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
chrome/common/extensions/docs/server2/echo_handler.py:68: self.response)
why can't you put *all* of this, besides main(), in its own file? like
handler.py.

kinda makes sense to move this file to appengine_main or something at some
point, too.

Anyway so there are 3 things we use from appengine: the launching stuff,
memcache, and urlfetch. The former is only used for launching the app, so it
would stay in here.

The latter two... I wonder if we can just fake it out, like, have a file
appengine_wrappers.py or something which either catches import exception on
appengine, or (somewhat) better exposes like a SetFakeMode or something method.

then files should have like

from appengine_wrappers import memcache
from appengine_wrappers import urlfetch

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:41:
self.assertEqual(200, response.status)
also assert that the response isn't empty?

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:46:
self.assertEqual(404, bad_response.status)
ditto

[cduvall, 2012-07-30 22:48:09]

This is still not testing warmup requests, because those require full appengine
functionality. They request pages on all of the branches, and I think to test
this we would have to mock out everything that it would be useful testing.

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
chrome/common/extensions/docs/server2/echo_handler.py:68: self.response)
On 2012/07/30 21:19:09, kalman wrote:
> why can't you put *all* of this, besides main(), in its own file? like
> handler.py.
> 
> kinda makes sense to move this file to appengine_main or something at some
> point, too.
> 
> Anyway so there are 3 things we use from appengine: the launching stuff,
> memcache, and urlfetch. The former is only used for launching the app, so it
> would stay in here.
> 
> The latter two... I wonder if we can just fake it out, like, have a file
> appengine_wrappers.py or something which either catches import exception on
> appengine, or (somewhat) better exposes like a SetFakeMode or something
method.
> 
> then files should have like
> 
> from appengine_wrappers import memcache
> from appengine_wrappers import urlfetch

Done.

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:41:
self.assertEqual(200, response.status)
On 2012/07/30 21:19:09, kalman wrote:
> also assert that the response isn't empty?

Done.

https://chromiumcodereview.appspot.com/10828042/diff/7001/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:46:
self.assertEqual(404, bad_response.status)
On 2012/07/30 21:19:09, kalman wrote:
> ditto

Done.

[not at google - send to devlin, 2012-07-30 23:00:44]

Cool, lgtm.

The idea with appengine_wrappers was for it to try to use fake implementations
if it can, so like, AppengineUrlFetcher doesn't need to have its urlfetch
function injected (please add comments to it too).

It kind of goes against the design of always injecting dependencies, but since
it makes the integration tests work with minimal contortions (and changes to the
real environment) I think it's a decent compromise.

And change get() to not be recursive :)

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
File chrome/common/extensions/docs/server2/appengine_url_fetcher.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:21:
self._urlfetch = urlfetch
I think you can revert the changes to this file and import appengine_wrappers
(that's the point?)

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
chrome/common/extensions/docs/server2/appengine_wrappers.py:11: urlfetch = None
it's kinda hacky, but you could avoid that awkward injection of the memcache
function by setting it to the fake implementation here.

set urlfetch to implementation that crashes if it's used?

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
chrome/common/extensions/docs/server2/handler.py:122: self.get()
recursively calling get..?

Wow I didn't even notice the way it did that already. There's got to be a more
sane way to do that, at least pull the stuff from line 131 onwards into a
different function and call it rather than get, or something.

[cduvall, 2012-07-30 23:38:18]

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
File chrome/common/extensions/docs/server2/appengine_url_fetcher.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
chrome/common/extensions/docs/server2/appengine_url_fetcher.py:21:
self._urlfetch = urlfetch
On 2012/07/30 23:00:44, kalman wrote:
> I think you can revert the changes to this file and import appengine_wrappers
> (that's the point?)

Done.

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
chrome/common/extensions/docs/server2/appengine_wrappers.py:11: urlfetch = None
On 2012/07/30 23:00:44, kalman wrote:
> it's kinda hacky, but you could avoid that awkward injection of the memcache
> function by setting it to the fake implementation here.
> 
> set urlfetch to implementation that crashes if it's used?

Done.

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/15001/chrome/common/exte...
chrome/common/extensions/docs/server2/handler.py:122: self.get()
On 2012/07/30 23:00:44, kalman wrote:
> recursively calling get..?
> 
> Wow I didn't even notice the way it did that already. There's got to be a more
> sane way to do that, at least pull the stuff from line 131 onwards into a
> different function and call it rather than get, or something.

Done.

[commit-bot: I haz the power, 2012-07-30 23:39:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/10828042/13

[commit-bot: I haz the power, 2012-07-30 23:40:18]

Presubmit check for 10828042-13 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
./integration_test.py failed!
Command ./integration_test.py returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2
ERROR:root:[Errno 2] No such file or directory:
'../../docs/server2/templates/public/junk.html'
.E
======================================================================
ERROR: testAll (__main__.IntegrationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./integration_test.py", line 36, in testAll
    Handler(request, response, local_path='../..').get()
  File
"/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2/handler.py",
line 133, in get
    self.redirect(path + '/')
AttributeError: 'Handler' object has no attribute 'redirect'

----------------------------------------------------------------------
Ran 2 tests in 6.927s

FAILED (errors=1)


Presubmit checks took 8.7s to calculate.

[commit-bot: I haz the power, 2012-07-30 23:46:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/10828042/6009

[commit-bot: I haz the power, 2012-07-31 01:07:46]

Change committed as 149101

[not at google - send to devlin, 2012-07-31 06:04:36]

Yep, sweet.

To test the warmup requests you can just create a MockRequest with '_ah/warmup'
as its path, right?

Then check that stuff has been warmed up, somehow. Worth doing in a different
test case.

https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_wrappers.py:7: # This will
attempt to import the actual Appengine modules, and if it fails,
nit: (here an below), it's "App Engine" not "Appengine".

https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
File chrome/common/extensions/docs/server2/branch_utility_test.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
chrome/common/extensions/docs/server2/branch_utility_test.py:16:
AppEngineMemcache(''))
yeah ok, I guess it'd be redundant to have two different in-memory
implementations. cool.

https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
File chrome/common/extensions/docs/server2/in_memory_memcache.py (right):

https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
chrome/common/extensions/docs/server2/in_memory_memcache.py:12: def set(self,
key, value, namespace, time=60):
this has different interface to AppEngineMemcache now, and it's kind of changed
meaning (now it's a fake interface to the appengine API, but previously it was a
fake interface to our own appengine wrapper).

So could you move it into appengine_wrapper.py?

[cduvall, 2012-07-31 17:56:35]

On 2012/07/31 06:04:36, kalman wrote:
> Yep, sweet.
> 
> To test the warmup requests you can just create a MockRequest with
'_ah/warmup'
> as its path, right?
> 
> Then check that stuff has been warmed up, somehow. Worth doing in a different
> test case.
> 
>
https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
> File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):
> 
>
https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
> chrome/common/extensions/docs/server2/appengine_wrappers.py:7: # This will
> attempt to import the actual Appengine modules, and if it fails,
> nit: (here an below), it's "App Engine" not "Appengine".
> 
>
https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
> File chrome/common/extensions/docs/server2/branch_utility_test.py (right):
> 
>
https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
> chrome/common/extensions/docs/server2/branch_utility_test.py:16:
> AppEngineMemcache(''))
> yeah ok, I guess it'd be redundant to have two different in-memory
> implementations. cool.
> 
>
https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
> File chrome/common/extensions/docs/server2/in_memory_memcache.py (right):
> 
>
https://chromiumcodereview.appspot.com/10828042/diff/6009/chrome/common/exten...
> chrome/common/extensions/docs/server2/in_memory_memcache.py:12: def set(self,
> key, value, namespace, time=60):
> this has different interface to AppEngineMemcache now, and it's kind of
changed
> meaning (now it's a fake interface to the appengine API, but previously it was
a
> fake interface to our own appengine wrapper).
> 
> So could you move it into appengine_wrapper.py?

Made these fixes in https://chromiumcodereview.appspot.com/10823105/