Extensions Docs Server: Testing GithubFileSystem and cron jobs

Extensions Docs Server: Testing GithubFileSystem and cron jobs

Test for GithubFileSystem and cron jobs.

BUG=141910
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152784

[not at google - send to devlin, 2012-08-22 13:57:58]

lgtm, just trivial comments.

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/appengine_wrappers.py:71: BLOBS = {}
_BLOBS?

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/appengine_wrappers.py:72: KEY = 1
can you put this on FakeFiles, and call it _next_blobstore_key or something?

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/appengine_wrappers.py:83: class
FakeStringIO(StringIO):
using just a normal StringIO isn't enough where this gets used?

Apart from that, wo comments: I'm sure that FakeStringIO can be given a better
name, and does this really need to subclass StringIO? The existence of this is
confusing, but I assume there's a good reason. Perhaps explain it.

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/appengine_wrappers.py:160: DB = {}
put on the db or Model class, call _store or something?

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/github_file_system.py (right):

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/github_file_system.py:32: if
self._old_version != self._new_version:
This "old version" vs "new version" thing is a bit weird when they can be the
same.

Would prefer if the arguments were like "key to set" and "key to delete
afterwards", the latter being optional.

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/github_file_system_test.py (right):

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/github_file_system_test.py:39:
self.assertEqual(0, self._file_system.Stat('zipball').version)
and these tests picked up the bug?

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10871002/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/handler.py:169: original_response =
self.response
rather than this, could you make _Render save then restore the request/response
objects that it clobbers?

[cduvall, 2012-08-22 17:20:43]

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/appengine_wrappers.py:71: BLOBS = {}
On 2012/08/22 13:57:58, kalman wrote:
> _BLOBS?

Done.

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/appengine_wrappers.py:72: KEY = 1
On 2012/08/22 13:57:58, kalman wrote:
> can you put this on FakeFiles, and call it _next_blobstore_key or something?

Done.

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/appengine_wrappers.py:83: class
FakeStringIO(StringIO):
On 2012/08/22 13:57:58, kalman wrote:
> using just a normal StringIO isn't enough where this gets used?
> 
> Apart from that, wo comments: I'm sure that FakeStringIO can be given a better
> name, and does this really need to subclass StringIO? The existence of this is
> confusing, but I assume there's a good reason. Perhaps explain it.

Done.

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/appengine_wrappers.py:160: DB = {}
On 2012/08/22 13:57:58, kalman wrote:
> put on the db or Model class, call _store or something?

Done.

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/github_file_system.py (right):

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/github_file_system.py:32: if
self._old_version != self._new_version:
On 2012/08/22 13:57:58, kalman wrote:
> This "old version" vs "new version" thing is a bit weird when they can be the
> same.
> 
> Would prefer if the arguments were like "key to set" and "key to delete
> afterwards", the latter being optional.

Done.

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/github_file_system_test.py (right):

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/github_file_system_test.py:39:
self.assertEqual(0, self._file_system.Stat('zipball').version)
On 2012/08/22 13:57:58, kalman wrote:
> and these tests picked up the bug?

Yeah these tests caught the error, but I just added an explicit key generation
test just to be thorough.

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/handler.py (right):

http://codereview.chromium.org/10871002/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/handler.py:169: original_response =
self.response
On 2012/08/22 13:57:58, kalman wrote:
> rather than this, could you make _Render save then restore the
request/response
> objects that it clobbers?

Done.