Docserver: SidenavDataSource refactor, transition to DataSourceRegistry

chrome/common/extensions/docs/server2/sidenav_data_source_test.py

Index: chrome/common/extensions/docs/server2/sidenav_data_source_test.py
diff --git a/chrome/common/extensions/docs/server2/sidenav_data_source_test.py b/chrome/common/extensions/docs/server2/sidenav_data_source_test.py
index e7eddc1724488d48e567a434c2c73a80b2c4d38a..50d24c022410f7694e9e66673472f8694bf5ee59 100755
--- a/chrome/common/extensions/docs/server2/sidenav_data_source_test.py
+++ b/chrome/common/extensions/docs/server2/sidenav_data_source_test.py
@@ -1,68 +1,148 @@
 #!/usr/bin/env python
-# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import json
-import os
-import sys
+import logging
 import unittest
 
 from compiled_file_system import CompiledFileSystem
-from local_file_system import LocalFileSystem
 from object_store_creator import ObjectStoreCreator
-from sidenav_data_source import SidenavDataSource
+from servlet import Request
+from sidenav_data_source import (
+    SidenavDataSource, _AddLevels, _AddSelected, _QualifyHrefs)
+from test_file_system import TestFileSystem
+
+
+class FakeServerInstance(object):
+  def __init__(self, file_system):
+    self.compiled_host_fs_factory = CompiledFileSystem.Factory(
+        file_system, ObjectStoreCreator.ForTest())
+    self.sidenav_json_base_path = ''
+
 
 class SamplesDataSourceTest(unittest.TestCase):
-  def setUp(self):
-    self._json_path = 'docs/server2/test_data/sidenav_data_source'
-    self._compiled_fs_factory = CompiledFileSystem.Factory(
-        LocalFileSystem.Create(),
-        ObjectStoreCreator.ForTest())
-
-  def _CheckLevels(self, items, level=2):
-    for item in items:
-      self.assertEqual(level, item['level'])
-      if 'items' in item:
-        self._CheckLevels(item['items'], level=level + 1)
-
-  def _ReadLocalFile(self, filename):
-    with open(os.path.join(sys.path[0],
-                           'test_data',
-                           'sidenav_data_source',
-                           filename), 'r') as f:
-      return f.read()
-
-  def testLevels(self):
-    sidenav_data_source = SidenavDataSource.Factory(self._compiled_fs_factory,
-                                                    self._json_path).Create('')
-    sidenav_json = sidenav_data_source.get('test')
-    self._CheckLevels(sidenav_json)
-
-  def testSelected(self):
-    sidenav_data_source = SidenavDataSource.Factory(
-        self._compiled_fs_factory,
-        self._json_path).Create('www.b.com')
-    sidenav_json = sidenav_data_source.get('test')
-    # This will be prettier once JSON is loaded with an OrderedDict.
-    for item in sidenav_json:
-      if item['title'] == 'Jim':
-        self.assertTrue(item.get('child_selected', False))
-        for next_item in item['items']:
-          if next_item['title'] == 'B':
-            self.assertTrue(next_item.get('selected', False))
-            return
-    # If we didn't return already, we should fail.
-    self.fail()
-
-  def testAbsolutePath(self):
-    sidenav_data_source = SidenavDataSource.Factory(
-        self._compiled_fs_factory,
-        self._json_path).Create('absolute_path/test.html')
-    sidenav_json = sidenav_data_source.get('absolute_path')
-    self.assertEqual(
-        sidenav_json,
-        json.loads(self._ReadLocalFile('absolute_path_sidenav_expected.json')))
+  def testAddLevels(self):
+    sidenav_json = [{
+      'title': 'H2',
+      'items': [{
+        'title': 'H3',
+        'items': [{ 'title': 'X1' }]
+      }]
+    }]
+
+    expected = [{
+      'level': 1,
+      'title': 'H2',
+      'items': [{
+        'level': 2,
+        'title': 'H3',
+        'items': [{ 'level': 3, 'title': 'X1' }]
+      }]
+    }]
+
+    _AddLevels(sidenav_json, 1)
+    self.assertEqual(expected, sidenav_json)
+
+  def testAddSelected(self):
+    sidenav_json = [
+      { 'href': '/AH2.html' },
+      {
+        'href': '/H2.html',
+        'items': [{
+          'href': '/H3.html'
+        }]
+      }
+    ]
+
+    expected = [
+      { 'href': '/AH2.html' },
+      {
+        'child_selected': True,
+        'href': '/H2.html',
+        'items': [{
+          'href': '/H3.html',
+          'selected': True
+        }]
+      }
+    ]
+
+    _AddSelected(sidenav_json, 'H3.html')
+    self.assertEqual(expected, sidenav_json)
+
+  def testQualifyHrefs(self):
+    sidenav_json = [
+      { 'href': '/qualified/H1.html' },
+      { 'href': 'https://qualified/X1.html' },
+      {
+        'href': 'H2.html',
+        'items': [{
+          'href': 'H3.html'
+        }]
+      }
+    ]
+
+    expected = [
+      { 'href': '/qualified/H1.html' },
+      { 'href': 'https://qualified/X1.html' },
+      {
+        'href': '/H2.html',
+        'items': [{
+          'href': '/H3.html'
+        }]
+      }
+    ]
+
+    logging.disable('warning')

[Jeffrey Yasskin, 2013/08/28 19:12:37]

We should test that the warnings are generated ... You could use
logging.getLogger('').addHandler(your_test_handler), and removeHandler after the
test, or you could import a library like
http://pythonhosted.org/testfixtures/logging.html, if we don't already have one.
We have pymock, but I don't see anything specific to logging.

[jshumway, 2013/08/29 00:13:09]

Okay, wasn't too sure what the best way to actually test the logging output was.

+    _QualifyHrefs(sidenav_json)
+    logging.disable(logging.NOTSET)
+
+    self.assertEqual(expected, sidenav_json)
+
+  def testSidenavDataSource(self):
+    file_system = TestFileSystem({
+      'apps_sidenav.json': json.dumps([{
+        'title': 'H1',
+        'href': 'H1.html',
+        'items': [{
+          'title': 'H2',
+          'href': '/H2.html'
+        }]
+      }])
+    })
+
+    expected = [{
+      'level': 2,
+      'child_selected': True,
+      'title': 'H1',
+      'href': '/H1.html',
+      'items': [{
+        'level': 3,
+        'selected': True,
+        'title': 'H2',
+        'href': '/H2.html'
+      }]
+    }]
+
+    sidenav_data_source = SidenavDataSource(
+        FakeServerInstance(file_system), Request.ForTest('/H2.html'))
+
+    logging.disable('warning')
+    self.assertEqual(expected, sidenav_data_source.get('apps'))
+    logging.disable(logging.NOTSET)
+
+  def testCron(self):
+    file_system = TestFileSystem({
+      'apps_sidenav.json': '[{ "title": "H1" }]' ,
+      'extensions_sidenav.json': '[{ "title": "H2" }]'
+    })
+
+    # Ensure Cron doesn't rely on request.
+    sidenav_data_source = SidenavDataSource(
+        FakeServerInstance(file_system), request=None)
+    sidenav_data_source.Cron()

[Jeffrey Yasskin, 2013/08/28 19:12:37]

Is there anything you can do to assert that Cron() actually loads the data it
should load?

[jshumway, 2013/08/29 00:13:09]

There is a bad way that requires lots of private variable accesses. There is
however precedent for it in my redirector patch
(https://codereview.chromium.org/22217004/diff/28001/chrome/common/extensions/...).

Though looking back at that patch, the only reason we left it like this was to
push the new server version quickly. I am going to add a new method to the
caches to check if a key exists in cache so that these checks don't have to be
as hacky. Added some TODOs.

[not at google - send to devlin, 2013/08/29 16:30:13]

We have an integration test which runs the cron job then renders every public
page in its offline state. This ensures that everything gets cached (of course -
there was that bug where the wrong file system was getting used - that would
also be catchable in an integration test come to think of it, by running a
different server).

Point is, private variable access is not a sustainable way to test this, nor a
complete one, and similarly inspecting the contents of the object store is
"slightly better" but still effectively testing an implementation detail of this
class.

Better would be to do a similar thing that the integration test does. For each
test, run the Cron method (you could even put it in SetUp but that may be overly
simplified). Then re-create the class in its offline configuration. So long as
the code coverage is good, this would be a good high-level test of the Cron
functionality.

+
 
 if __name__ == '__main__':
   unittest.main()