[chromedriver] setting browser default download directory

chrome/test/chromedriver/test/run_py_tests.py

Index: chrome/test/chromedriver/test/run_py_tests.py
diff --git a/chrome/test/chromedriver/test/run_py_tests.py b/chrome/test/chromedriver/test/run_py_tests.py
index 2b106a6783a372544520a9fcae0f182b673be9e2..1ff3472513ec99cc4706c4cae096b0361130b91f 100755
--- a/chrome/test/chromedriver/test/run_py_tests.py
+++ b/chrome/test/chromedriver/test/run_py_tests.py
@@ -152,7 +152,7 @@ class ChromeDriverBaseTest(unittest.TestCase):
       except:
         pass
 
-  def CreateDriver(self, server_url=None, **kwargs):
+  def CreateDriver(self, server_url=None, download_dir=None, **kwargs):
     if server_url is None:
       server_url = _CHROMEDRIVER_SERVER_URL
 
@@ -165,7 +165,7 @@ class ChromeDriverBaseTest(unittest.TestCase):
         android_activity = constants.PACKAGE_INFO[_ANDROID_PACKAGE_KEY].activity
         android_process = '%s:main' % android_package
 
-    driver = chromedriver.ChromeDriver(server_url,
+    driver = chromedriver.ChromeDriver(server_url, download_dir,

[samuong, 2014/10/16 20:23:41]

Put each parameter on a separate line.

[andrewcheng, 2014/10/17 22:59:54]

Done.

                                        chrome_binary=_CHROME_BINARY,
                                        android_package=android_package,
                                        android_activity=android_activity,
@@ -203,11 +203,43 @@ class ChromeDriverTest(ChromeDriverBaseTest):
     return ChromeDriverTest._http_server.GetUrl() + file_path
 
   def setUp(self):
-    self._driver = self.CreateDriver()
+    if self._testMethodName == 'testFileDownLoad':

[samuong, 2014/10/16 20:23:40]

As we discussed earlier, I don't think this is a very good way to set up the
download directory. Let's move testFileDownLoad into the ChromeDownloadDirTest
class so that all the download-related tests are together. Please revert
ChromeDriverTest.setUp() and ChromeDriverTest.tearDown() and see my comment
under ChromeDownloadDirTest for some pointers on how to do this.

+       self.download_dir = tempfile.mkdtemp()
+    else:
+       self.download_dir =  None
+    self._driver = self.CreateDriver(download_dir=self.download_dir)
+
+  def tearDown(self):
+    try:
+      if self.download_dir:
+        os.removedirs(self.download_dir)
+        self.download_dir = None
+    except:
+      self.assertRaises(chromedriver.UnknownError)
+    ChromeDriverBaseTest.tearDown(self)
 
   def testStartStop(self):
     pass
 
+  def testFileDownLoad(self):
+    # check download file to make sure it does  not exist
+    download_name = os.path.join(self.download_dir, 'a_red_dot.png')
+    self.assertFalse(os.path.exists(download_name),
+       "Test prefer download directory failed! - download file already exist")

[samuong, 2014/10/16 20:23:40]

I think you were right in your previous comment that this assertion is
unnecessary, since it comes right after the temporary directory is created.
Let's just remove it.

[andrewcheng, 2014/10/17 22:59:54]

Done.

+
+    self._driver.Load(self.GetHttpUrlForFile('/chromedriver/download.html'))
+    self._driver.FindElement('id', 'red-dot').Click()
+    # wait until download completed
+    timeout = time.time() + 60*1   # 1 minutes from now

[samuong, 2014/10/16 20:23:40]

I think these two comments state the obvious, and are not really necessary.

Also, don't forget to put spaces before and after the '*'.

Also, weren't we going to rename 'timeout' to 'deadline'?

[andrewcheng, 2014/10/17 22:59:53]

another place  also has timeout
def _WaitForNewWindow(self, old_handles):
Line 254
 timeout = time.time() + 20
    while time.time() < timeout:

 should we change it as well - consist

+    while True:
+       time.sleep(0.1)
+       if os.path.isfile(download_name) or time.time() > timeout:
+             break
+
+    self.assertTrue(os.path.isfile(download_name),
+            "Test user prefer download directory failed!")

[samuong, 2014/10/16 20:23:40]

I don't think this sentence is grammatically correct. How about we change it to
'Failed to download file'?

[andrewcheng, 2014/10/17 22:59:53]

 good  idea

+    os.remove(download_name)
+
   def testLoadUrl(self):
     self._driver.Load(self.GetHttpUrlForFile('/chromedriver/empty.html'))
 
@@ -750,6 +782,39 @@ class ChromeDriverAndroidTest(ChromeDriverBaseTest):
     self._drivers[0].Quit()
     self._drivers[0] = self.CreateDriver()
 
+class ChromeDownloadDirTest(ChromeDriverBaseTest):

[samuong, 2014/10/16 20:23:41]

Add the following functions here to set up the download directory:

  def setUp(self):
    self._http_server = webserver.WebServer(chrome_paths.GetTestData())
    self._download_dir = tempfile.mkdtemp()
    self._driver = self.CreateDriver(download_dir=self._download_dir)

  def tearDown(self):
    os.rmdir(self._download_dir)
    self._http_server.Shutdown()
    ChromeDriverBaseTest.tearDown(self)

+
+  # test existing  prefence profile - check setting if it is correct

[samuong, 2014/10/16 20:23:41]

Use a docstring here rather than a regular comment (see the other lines  that
start and end with three double-quotes """ for examples).

[andrewcheng, 2014/10/17 22:59:54]

Done.

+  def testDownloadDirectoryOverridesExistingPreferences(self):
+    user_data_dir = tempfile.mkdtemp()
+    tmp_download_dir = tempfile.mkdtemp()

[samuong, 2014/10/16 20:23:40]

If you use the setUp method I mentioned above, you won't need to do this.

[andrewcheng, 2014/10/17 22:59:54]

 not sure how this work, i don't touch for now

+    sub_dir = os.path.join(user_data_dir, 'Default')
+    os.mkdir(sub_dir)
+    def_pref_file = os.path.join(sub_dir, 'Preferences')

[samuong, 2014/10/16 20:23:41]

How about we call this "prefs_file_path"?

[andrewcheng, 2014/10/17 22:59:54]

Done.

+
+    data = {

[samuong, 2014/10/16 20:23:41]

"data" is not a very descriptive name, how about we call it "prefs"?

[andrewcheng, 2014/10/17 22:59:53]

Done.

+      'test': 'this should not be changed',
+      'download': {
+        'directory_upgrade': 'true',

[samuong, 2014/10/16 20:23:40]

As I mentioned somewhere else in this review, I think 'directory_upgrade' might
not need to be specified. If it isn't needed, please remove it.

[andrewcheng, 2014/10/17 22:59:54]

Done.

+        'default_directory': '/old/download/directory'
+      }
+    }
+
+    with open(def_pref_file, 'w') as outfile:

[samuong, 2014/10/16 20:23:41]

"outfile" is not very descriptive, how about we call this "prefs_file"?

[andrewcheng, 2014/10/17 22:59:53]

 this is  generic use for short term - as temp,  so that we know it is for
writing. (as documentation)

 we have 
    with open(prefs_file_path, 'w') as outfile:
 
  this is easier to read

+      json.dump(data, outfile)
+
+    driver = self.CreateDriver(chrome_switches=
+            ['user-data-dir=' + user_data_dir],
+            download_dir = tmp_download_dir)
+
+    with open(def_pref_file) as infile:

[samuong, 2014/10/16 20:23:40]

Same as "outfile" - how about we call this "prefs_file"?

[andrewcheng, 2014/10/17 22:59:52]

 same comment provide in outfile

+        data = json.load(infile)
+    self.assertEqual(data['test'], 'this should not be changed',
+                      "The test data is drop - should stay without change")

[samuong, 2014/10/16 20:23:41]

I don't think this error message is grammatically correct. How about we change
it to something like: 'Existing preference was unexpectedly overridden'?

[andrewcheng, 2014/10/17 22:59:54]

  it will not be set to other  value - only disappear.
  may be we need better description than
"The test data is drop - should stay without change"

 I changed to
"The test data disappear - it should stay without change"
for now

+    download = data['download']
+    self.assertEqual(download['default_directory'],
+         tmp_download_dir,
+          'default_directory is not set')

[samuong, 2014/10/16 20:23:40]

This is a bit misleading. The default_directory value will be set, but might not
be set to the correct value. How about we change it to the following?

'Download directory preference was not updated to match capabilities'

[andrewcheng, 2014/10/17 22:59:54]

Done.

 
 class ChromeSwitchesCapabilityTest(ChromeDriverBaseTest):
   """Tests that chromedriver properly processes chromeOptions.args capabilities.