Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7541046:
Adding pyauto tests for the NaCl SDK. (Closed)
|
Created:
9 years, 4 months ago by chrisphan Modified:
9 years, 4 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionAdding pyauto tests for the NaCl SDK.
Automation for step 1-9 testplan @ go/NACL-SDK-testing
BUG=89467, 89470, 89479, 89610, 89612
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96230
Patch Set 1 #
Total comments: 6
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 139
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 34
Patch Set 6 : '' #
Total comments: 4
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Messages
Total messages: 16 (0 generated)
Did not go through the whole CL yet. comments until now: http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py#newcode28 functional/nacl_sdk.py:28: _SEVENZIP_PATH = 'C:\\Program Files\\7-Zip\\7z.exe' its not guaranteed that C drive will be available on all machines where these tests will be run. hence do something like this: os.path.join(os.getenv('ProgramFiles'), r"7-Zip"). Also, it is not sure 7-zip will be available on all machines. please write code to handle that case. http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py#newcode36 functional/nacl_sdk.py:36: #self._VerifyNaClPlugin() Do we have bug for this? http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py#newcode101 functional/nacl_sdk.py:101: def _VerifyDownloadLinks(self): Can you first detect current platform and then verify download links? Looks like the following code is going to check for all platforms all the time.
http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py#newcode28 functional/nacl_sdk.py:28: _SEVENZIP_PATH = 'C:\\Program Files\\7-Zip\\7z.exe' Yes, it is handled. I was told the bot does have 7-Zip installed. But in case they don't, 7-Zip should be in the pyauto third-party folder somewhere. 7-Zip is likely to be in the 64bit program directory which is "Program File", while getenv will give the x86 one. I'll check other options. http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py#newcode36 functional/nacl_sdk.py:36: #self._VerifyNaClPlugin() Some of these tests verify the live examples. And they were compiled by an older sdk so they only work on Chrome 10-13. So until they're updated, these test will fail if running on Chrome 14. http://codereview.chromium.org/7541046/diff/1/functional/nacl_sdk.py#newcode101 functional/nacl_sdk.py:101: def _VerifyDownloadLinks(self): This is for step 2. I believe we just need to make sure the URL is under the right OS label. But yes I can separate it.
http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:31: if self._HasAllSystemRequirements() == False: if not self._HasAllSystemRequirements(): http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:31: if self._HasAllSystemRequirements() == False: This check is performed before all 3 of the tests in this file. We should probably put this check inside a setUp() function, which will automatically be called before each test is invoked. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:32: return Before returning, maybe we can log a message indicating that the current system doesn't match the requirements for these tests. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:48: self._RemoveDownloadedTestFile() I recommend removing the blank lines in-between each of the statements in lines 34-48 above. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:69: file_path = os.path.join(download_dir, 'naclsdk_linux.tgz') maybe add an "else:" in which we fail (just to be safe). http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:77: msg='Unexpected checksum. Expected: %s, get: %s' nit: 'get' --> 'got' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:90: self._OpenExamplesAndStartTest(examples) We could combine the above 3 lines: self._OpenExamplesAndStartTest( self._GetTestSetting()['prerelease_gallery']) http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:96: self._OpenExamplesAndStartTest(examples) We could combine the above 3 lines like we did in line 90 above. Then, since this function is only called from one place, it's probably not worth defining this as a separate function; we can just inline the code where it's needed. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:102: self.NavigateToURL(sdk_download_url) Combine the above 2 lines: self.NavigateToURL(settings['post_sdk_download_url']) http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:113: msg='Misplace download link: %s' % win_sdk_url) nit: 'Misplace' --> 'Misplaced". Same at lines 121 and 129 below. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:129: msg='Misplace download link: %s' % lin_sdk_url) Maybe add an "else:" case in which we fail (just to be safe) http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:197: # Build a c project. nit: 'c' --> 'C' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:208: # Build a cc project. nit: 'cc' --> 'C++' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:217: msg='Cannot build hello_cc stub project.') Lines 197-206 and lines 208-217 are almost identical, except for a few minor differences. Maybe we can look into defining a helper function (within the current function) that we can specify once, and call twice, to handle both of these cases. That way, we avoid duplicated code. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:226: if not self._IsURLAlive('http://localhost:5103'): Shouldn't we remove the "not" here? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:237: self.NavigateToURL('http://localhost:5103') Are we sure the server has started by the time we get to this point? It looks like the server is being launched at either line 232 or 235 above, but we don't do anything to wait until the server has started (and is ready to serve pages) by the time we get to this point. Could this introduce flaky behavior in the tests? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:237: self.NavigateToURL('http://localhost:5103') Also, should we gracefully handle the case when port 5103 may already be bound on the current machine by some other process than our own local server? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:256: """Re-build the examples.""" 'Re-build the examples and verify they are as expected.' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:294: examples_path = self._GetDirectoryPath('examples', sdk_path) A lot of the helper functions in this file start with the above 3 lines. Maybe we should define the above 3 variables once, and pass them around to the helper functions, to avoid having to re-define them in all of these helper functions? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:339: success = False We might be able to remove this line. I think the if/else below guarantees that "success" will be defined before line 346 executes. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:349: # Run the appropriate ncval for your platform on the matching .nexe. 'your platform' --> 'the current platform' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:352: success = False Same comment as line 339 above. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:362: success = False Same comment as line 339 above. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:374: extracted_sdk_path = os.path.join(download_dir, self._EXTRACTED_NACL_SDK_DIR) This line is just a little bit too long. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:382: pyauto_utils.RemovePath(sdk_path) We can shorten the above 6 lines: for sdk_path in ['naclsdk_win.exe', 'naclsdk_mac.tgz', 'naclsdk_linux.tgz']: pyauto_utils.RemovePath(os.path.join(download_dir, sdk_path) http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:391: False if checking if param look_out is not in output. 'look_out' --> 'look_for' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:394: True, if output contains parameter look_for, or 'True, if output contains parameter look_for,' --> 'True, if output contains parameter |look_for| and |is_in| is True,' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:406: return False Change the above 4 lines to this: return is_in http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:414: return False Lines 400-406 and 408-414 are also very similar. Maybe look into defining a little helper function (within the current function) that does this work once, but is called twice, to remove duplicate code. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:418: return True Change the above 4 lines to this: return not is_in http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:439: for _ in range(1): Doesn't this just execute once? Why is this loop needed? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:450: # examples operate correctly. Rather than randomly closing tabs, it might be better to use a pre-defined sequence of tabs to close. That way, the test case is deterministic. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:471: """Verify NaCl Example is working. nit: 'Example' --> 'example' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:498: if tab_index != None: I think this should work too: if tab_index: http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:509: wait_for = 60 Maybe no need to define this as a separate variable. Just inline the value where needed at line 514 below. Same comment in the other helpers below. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:514: wait_for, expect_retval='SUCCESS') timeout=wait_for http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:549: # Get top corner of pi image. nit: 'pi' --> 'Pi' to make it consistent with line 536 above. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:645: wait_for = 30 Similar comment as line 509 above. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:648: wait_for, expect_retval=16777215) timeout=wait_for http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:657: width: Width of the area to look scan. Remove 'look'? If you do so, make the same change where applicable below. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:660: retry_sleep: Sleep time in between each tries. nits: 'in between' --> 'in-between' 'tries' --> 'try' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:684: return False Change the above 4 lines to this: return pyauto.PyUITest.IsWin() http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:687: """Get an area of pixel color and return a list. 'a list' --> 'a list of color code values' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:701: return self._GetAreaPixelColorWin(x, y, width, height) As you implement these helpers for the other platforms, you may want to consider using just a single helper, which contains 3 cases for each platform (as opposed to three separate helper functions). This will save the space of having to write a docstring for each separate helper function for each platform. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:739: return self._GetPixelWin(x, y) Similar comment as line 701 above. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:772: # Verify there're no crash dump files nit: add period at end of sentence. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:775: msg='Crash dump %s found' % dmp_file) indent this line so that the parameter lines up underneath the first parameter in the previous line http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:781: if self.GetDOMValue('document.body.innerHTML', 0, tab['index']) == '': if not self.GetDOMValue('document.body.innerHTML', 0, tab['index']): http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:785: # TODO(chrisphan): handle specific action: close browser, close tab nit: add periods at the end of the above 2 lines. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:789: pass Maybe add an "else" case that gracefully handles an unexpected last_action? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:795: last_action: Specify action taken before checking for crashes. 'Specify' --> 'Last' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:814: """Check if file exist in directory. 'Check if a file matching the specified pattern exists in a directory.' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:826: return False Change the above 3 lines to this: return len(glob.glob(os.path.join(root, pattern))) > 0 http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:838: """ This docstring looks the same as for the function above, but that can't be right - otherwise there's no reason for 2 different functions. Please ensure the docstrings clarify what is different above this function as compared to the above function. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:858: return False This function is almost exactly the same as for the function above, except here we use "dirs" instead of "files". Why not combine the above 2 functions into a single function, and pass an extra parameter that specifies whether we want "files" or "dirs"? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:877: """Verify NaCl SDK Installation system requirements. nit: 'Installation' --> 'installation' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:902: # Nacl supports Chrome 10 and higher builds. nit: 'Nacl' --> 'NaCl' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:911: return True Change lines 908-911 to this: return chrome_build >= min_required_chrome_build http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:918: lin_sdk_url = settings['release_lin_sdk_url'] Remove the above 3 lines, and just inline the values where needed at lines 923, 926, and 929 below. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:942: Remove 1 of these blank lines. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:949: if not os.path.exists(destination): If we remove the directory in line 948 above, why check here to make sure it does not exist? Maybe if the directory still exists, we should issue an error saying that the old directory could not be cleaned out? Or, maybe we should clean out the directory as part of a "tearDown()" step after each test is run, so that we can assume the directory doesn't already exist when a new test starts? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:956: msg='7-Zip is required but could not be found.') indent this line by 4 more spaces http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:961: stdout=subprocess.PIPE) Indent these 2 lines by only 4 spaces as compared to the previous line: proc = subprocess.Popen( [seven_z_file_path, 'x', source_file, '-o' + destination], stdout=subprocess.PIPE) http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:974: def _Get7ZipPath(self): If this function only applies to Windows, maybe we should add 'Win' to the function name. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:979: None if not exist. 'String path to the 7-Zip executable, or None if it cannot be found.' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1032: pass If we get to this 'except' clause, does that mean the server was likely not killed? If so, should we issue an error in this case? http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1064: """Search NaCl SDK file for example files and directories. add to this docstring: 'in Windows' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1079: msg='7-Zip is required but could not be found.') indent this line more so that it lines up underneath the first parameter in the previous line. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1098: """"Enable NaCl Plugin.""" nit: 'Plugin' --> 'plugin' http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1100: if len(nacl_plugin) == 0: if not nacl_plugin: http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1102: if len(nacl_plugin) == 0: if not nacl_plugin: http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1103: self.fail(msg='No NaCl Plugin found.') nit: 'Plugin' --> 'plugin'
http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:32: return On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Before returning, maybe we can log a message indicating that the current system > doesn't match the requirements for these tests. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:48: self._RemoveDownloadedTestFile() On 2011/08/08 18:00:53, dennis_jeffrey wrote: > I recommend removing the blank lines in-between each of the statements in lines > 34-48 above. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:69: file_path = os.path.join(download_dir, 'naclsdk_linux.tgz') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > maybe add an "else:" in which we fail (just to be safe). Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:77: msg='Unexpected checksum. Expected: %s, get: %s' On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'get' --> 'got' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:90: self._OpenExamplesAndStartTest(examples) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > We could combine the above 3 lines: > > self._OpenExamplesAndStartTest( > self._GetTestSetting()['prerelease_gallery']) Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:96: self._OpenExamplesAndStartTest(examples) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > We could combine the above 3 lines like we did in line 90 above. Then, since > this function is only called from one place, it's probably not worth defining > this as a separate function; we can just inline the code where it's needed. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:102: self.NavigateToURL(sdk_download_url) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Combine the above 2 lines: > > self.NavigateToURL(settings['post_sdk_download_url']) Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:113: msg='Misplace download link: %s' % win_sdk_url) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'Misplace' --> 'Misplaced". Same at lines 121 and 129 below. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:129: msg='Misplace download link: %s' % lin_sdk_url) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Maybe add an "else:" case in which we fail (just to be safe) Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:197: # Build a c project. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'c' --> 'C' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:208: # Build a cc project. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'cc' --> 'C++' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:217: msg='Cannot build hello_cc stub project.') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Lines 197-206 and lines 208-217 are almost identical, except for a few minor > differences. Maybe we can look into defining a helper function (within the > current function) that we can specify once, and call twice, to handle both of > these cases. That way, we avoid duplicated code. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:226: if not self._IsURLAlive('http://localhost:5103'): On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Shouldn't we remove the "not" here? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:237: self.NavigateToURL('http://localhost:5103') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Are we sure the server has started by the time we get to this point? It looks > like the server is being launched at either line 232 or 235 above, but we don't > do anything to wait until the server has started (and is ready to serve pages) > by the time we get to this point. Could this introduce flaky behavior in the > tests? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:237: self.NavigateToURL('http://localhost:5103') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Also, should we gracefully handle the case when port 5103 may already be bound > on the current machine by some other process than our own local server? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:256: """Re-build the examples.""" On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'Re-build the examples and verify they are as expected.' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:339: success = False On 2011/08/08 18:00:53, dennis_jeffrey wrote: > We might be able to remove this line. I think the if/else below guarantees that > "success" will be defined before line 346 executes. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:349: # Run the appropriate ncval for your platform on the matching .nexe. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'your platform' --> 'the current platform' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:352: success = False On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Same comment as line 339 above. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:362: success = False On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Same comment as line 339 above. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:374: extracted_sdk_path = os.path.join(download_dir, self._EXTRACTED_NACL_SDK_DIR) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > This line is just a little bit too long. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:382: pyauto_utils.RemovePath(sdk_path) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > We can shorten the above 6 lines: > > for sdk_path in ['naclsdk_win.exe', 'naclsdk_mac.tgz', 'naclsdk_linux.tgz']: > pyauto_utils.RemovePath(os.path.join(download_dir, sdk_path) Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:391: False if checking if param look_out is not in output. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'look_out' --> 'look_for' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:394: True, if output contains parameter look_for, or On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'True, if output contains parameter look_for,' --> > > 'True, if output contains parameter |look_for| and |is_in| is True,' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:406: return False On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Change the above 4 lines to this: > > return is_in Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:418: return True On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Change the above 4 lines to this: > > return not is_in Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:439: for _ in range(1): On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Doesn't this just execute once? Why is this loop needed? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:450: # examples operate correctly. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Rather than randomly closing tabs, it might be better to use a pre-defined > sequence of tabs to close. That way, the test case is deterministic. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:471: """Verify NaCl Example is working. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'Example' --> 'example' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:509: wait_for = 60 On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Maybe no need to define this as a separate variable. Just inline the value > where needed at line 514 below. Same comment in the other helpers below. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:514: wait_for, expect_retval='SUCCESS') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > timeout=wait_for Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:549: # Get top corner of pi image. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'pi' --> 'Pi' to make it consistent with line 536 above. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:645: wait_for = 30 On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Similar comment as line 509 above. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:648: wait_for, expect_retval=16777215) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > timeout=wait_for Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:657: width: Width of the area to look scan. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Remove 'look'? If you do so, make the same change where applicable below. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:660: retry_sleep: Sleep time in between each tries. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nits: > 'in between' --> 'in-between' > 'tries' --> 'try' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:684: return False On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Change the above 4 lines to this: > > return pyauto.PyUITest.IsWin() Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:701: return self._GetAreaPixelColorWin(x, y, width, height) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > As you implement these helpers for the other platforms, you may want to consider > using just a single helper, which contains 3 cases for each platform (as opposed > to three separate helper functions). This will save the space of having to > write a docstring for each separate helper function for each platform. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:739: return self._GetPixelWin(x, y) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Similar comment as line 701 above. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:772: # Verify there're no crash dump files On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: add period at end of sentence. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:775: msg='Crash dump %s found' % dmp_file) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > indent this line so that the parameter lines up underneath the first parameter > in the previous line Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:781: if self.GetDOMValue('document.body.innerHTML', 0, tab['index']) == '': On 2011/08/08 18:00:53, dennis_jeffrey wrote: > if not self.GetDOMValue('document.body.innerHTML', 0, tab['index']): Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:785: # TODO(chrisphan): handle specific action: close browser, close tab On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: add periods at the end of the above 2 lines. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:789: pass On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Maybe add an "else" case that gracefully handles an unexpected last_action? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:795: last_action: Specify action taken before checking for crashes. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'Specify' --> 'Last' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:814: """Check if file exist in directory. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'Check if a file matching the specified pattern exists in a directory.' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:826: return False On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Change the above 3 lines to this: > > return len(glob.glob(os.path.join(root, pattern))) > 0 Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:838: """ On 2011/08/08 18:00:53, dennis_jeffrey wrote: > This docstring looks the same as for the function above, but that can't be right > - otherwise there's no reason for 2 different functions. Please ensure the > docstrings clarify what is different above this function as compared to the > above function. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:858: return False On 2011/08/08 18:00:53, dennis_jeffrey wrote: > This function is almost exactly the same as for the function above, except here > we use "dirs" instead of "files". Why not combine the above 2 functions into a > single function, and pass an extra parameter that specifies whether we want > "files" or "dirs"? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:877: """Verify NaCl SDK Installation system requirements. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'Installation' --> 'installation' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:902: # Nacl supports Chrome 10 and higher builds. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'Nacl' --> 'NaCl' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:918: lin_sdk_url = settings['release_lin_sdk_url'] On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Remove the above 3 lines, and just inline the values where needed at lines 923, > 926, and 929 below. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:942: On 2011/08/08 18:00:53, dennis_jeffrey wrote: > Remove 1 of these blank lines. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:949: if not os.path.exists(destination): On 2011/08/08 18:00:53, dennis_jeffrey wrote: > If we remove the directory in line 948 above, why check here to make sure it > does not exist? Maybe if the directory still exists, we should issue an error > saying that the old directory could not be cleaned out? Or, maybe we should > clean out the directory as part of a "tearDown()" step after each test is run, > so that we can assume the directory doesn't already exist when a new test > starts? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:956: msg='7-Zip is required but could not be found.') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > indent this line by 4 more spaces Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:974: def _Get7ZipPath(self): On 2011/08/08 18:00:53, dennis_jeffrey wrote: > If this function only applies to Windows, maybe we should add 'Win' to the > function name. Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:979: None if not exist. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'String path to the 7-Zip executable, or None if it cannot be found.' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1032: pass On 2011/08/08 18:00:53, dennis_jeffrey wrote: > If we get to this 'except' clause, does that mean the server was likely not > killed? If so, should we issue an error in this case? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1064: """Search NaCl SDK file for example files and directories. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > add to this docstring: 'in Windows' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1098: """"Enable NaCl Plugin.""" On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'Plugin' --> 'plugin' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1103: self.fail(msg='No NaCl Plugin found.') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > nit: 'Plugin' --> 'plugin' Done.
http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:31: if self._HasAllSystemRequirements() == False: As for Python 2.6, I don't see a way to skip a test without raising an error or failure in setUp(). Some system may not be intended for the test. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:31: if self._HasAllSystemRequirements() == False: On 2011/08/08 18:00:53, dennis_jeffrey wrote: > if not self._HasAllSystemRequirements(): Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:294: examples_path = self._GetDirectoryPath('examples', sdk_path) On 2011/08/08 18:00:53, dennis_jeffrey wrote: > A lot of the helper functions in this file start with the above 3 lines. Maybe > we should define the above 3 variables once, and pass them around to the helper > functions, to avoid having to re-define them in all of these helper functions? Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:498: if tab_index != None: On 2011/08/08 18:00:53, dennis_jeffrey wrote: > I think this should work too: > > if tab_index: Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:687: """Get an area of pixel color and return a list. On 2011/08/08 18:00:53, dennis_jeffrey wrote: > 'a list' --> 'a list of color code values' Done. http://codereview.chromium.org/7541046/diff/5001/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1079: msg='7-Zip is required but could not be found.') On 2011/08/08 18:00:53, dennis_jeffrey wrote: > indent this line more so that it lines up underneath the first parameter in the > previous line. Done.
Several more comments. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:36: if self._HasAllSystemRequirements() == False: if not self._HasAllSystemRequirements(): http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:225: sucess = self.WaitUntil( nit: 'sucess' --> 'success' http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:227: timeout=5, retry_sleep=1, expect_retval=True) Do you think a timeout of 5 seconds is enough? If the local machine is slow, perhaps it could take longer. Would there be any problem if we used a timeout of, say, 30 seconds? It seems a little safer to me. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:369: 'naclsdk_linux.tgz']: nit: indent this line by 5 fewer spaces. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:476: if tab_index != None: I think this should work too: if tab_index: http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:621: wait_for = 30 Maybe no need to define this as a separate variable. Just inline the value where needed at line 624 below. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:624: wait_for, expect_retval=16777215) 'wait_for' --> 'timeout=30' http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:745: # Verify there're no crash dump files.s remove the 's' at the end of this line. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:754: if self.GetDOMValue('document.body.innerHTML', 0, tab['index']) == '': if not self.GetDOMValue('document.body.innerHTML', 0, tab['index']): http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:803: in a directory and subdirectories. This first line in the docstring should be a 1-line summary. How about this: """Recursively checks if a file/directory matching a pattern exists.""" http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:816: if len(fnmatch.filter(files, pattern)) > 0: I think this would be equivalent if we remove the '> 0'. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:819: if len(fnmatch.filter(dirs, pattern)) > 0: I think this would be equivalent if we remove the '> 0'. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:874: return True Change lines 871-874 to this: return chrome_build >= min_required_chrome_build http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:987: self.fail(msg='Fail to close HTTP server') nit: 'fail' --> 'failed' http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1053: """"Enable NaCl Plugin.""" nit: 'Plugin' --> 'plugin' http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1055: if len(nacl_plugin) == 0: if not nacl_plugin: http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1057: if len(nacl_plugin) == 0: if not nacl_plugin:
http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:36: if self._HasAllSystemRequirements() == False: On 2011/08/09 17:14:21, dennis_jeffrey wrote: > if not self._HasAllSystemRequirements(): Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:225: sucess = self.WaitUntil( On 2011/08/09 17:14:21, dennis_jeffrey wrote: > nit: 'sucess' --> 'success' Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:227: timeout=5, retry_sleep=1, expect_retval=True) On 2011/08/09 17:14:21, dennis_jeffrey wrote: > Do you think a timeout of 5 seconds is enough? If the local machine is slow, > perhaps it could take longer. Would there be any problem if we used a timeout > of, say, 30 seconds? It seems a little safer to me. Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:369: 'naclsdk_linux.tgz']: On 2011/08/09 17:14:21, dennis_jeffrey wrote: > nit: indent this line by 5 fewer spaces. Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:476: if tab_index != None: On 2011/08/09 17:14:21, dennis_jeffrey wrote: > I think this should work too: > > if tab_index: Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:621: wait_for = 30 On 2011/08/09 17:14:21, dennis_jeffrey wrote: > Maybe no need to define this as a separate variable. Just inline the value > where needed at line 624 below. Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:624: wait_for, expect_retval=16777215) On 2011/08/09 17:14:21, dennis_jeffrey wrote: > 'wait_for' --> 'timeout=30' Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:745: # Verify there're no crash dump files.s On 2011/08/09 17:14:21, dennis_jeffrey wrote: > remove the 's' at the end of this line. Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:754: if self.GetDOMValue('document.body.innerHTML', 0, tab['index']) == '': On 2011/08/09 17:14:21, dennis_jeffrey wrote: > if not self.GetDOMValue('document.body.innerHTML', 0, tab['index']): Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:803: in a directory and subdirectories. On 2011/08/09 17:14:21, dennis_jeffrey wrote: > This first line in the docstring should be a 1-line summary. How about this: > > """Recursively checks if a file/directory matching a pattern exists.""" Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:816: if len(fnmatch.filter(files, pattern)) > 0: On 2011/08/09 17:14:21, dennis_jeffrey wrote: > I think this would be equivalent if we remove the '> 0'. Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:819: if len(fnmatch.filter(dirs, pattern)) > 0: On 2011/08/09 17:14:21, dennis_jeffrey wrote: > I think this would be equivalent if we remove the '> 0'. Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:874: return True On 2011/08/09 17:14:21, dennis_jeffrey wrote: > Change lines 871-874 to this: > > return chrome_build >= min_required_chrome_build Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:987: self.fail(msg='Fail to close HTTP server') On 2011/08/09 17:14:21, dennis_jeffrey wrote: > nit: 'fail' --> 'failed' Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1053: """"Enable NaCl Plugin.""" On 2011/08/09 17:14:21, dennis_jeffrey wrote: > nit: 'Plugin' --> 'plugin' Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1055: if len(nacl_plugin) == 0: On 2011/08/09 17:14:21, dennis_jeffrey wrote: > if not nacl_plugin: Done. http://codereview.chromium.org/7541046/diff/13001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1057: if len(nacl_plugin) == 0: On 2011/08/09 17:14:21, dennis_jeffrey wrote: > if not nacl_plugin: Done.
LGTM Just 2 small typos to fix before submitting. Thanks for addressing all those comments! http://codereview.chromium.org/7541046/diff/14001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/14001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1050: if len(nacl_plugin): Oops - we need a 'not' here: if not len(nacl_plugin): http://codereview.chromium.org/7541046/diff/14001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1052: if len(nacl_plugin): Oops - we need a 'not' here: if not len(nacl_plugin):
http://codereview.chromium.org/7541046/diff/14001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7541046/diff/14001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1050: if len(nacl_plugin): On 2011/08/09 18:32:15, dennis_jeffrey wrote: > Oops - we need a 'not' here: > > if not len(nacl_plugin): Done. http://codereview.chromium.org/7541046/diff/14001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:1052: if len(nacl_plugin): On 2011/08/09 18:32:15, dennis_jeffrey wrote: > Oops - we need a 'not' here: > > if not len(nacl_plugin): Done.
LGTM Thanks!
Presubmit check for 7541046-17002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/test/functional/nacl_sdk.py, line 913, 81 chars Found line ending with white spaces in: *************** chrome/test/functional/nacl_sdk.py, line 29 chrome/test/functional/nacl_sdk.py, line 33 chrome/test/functional/nacl_sdk.py, line 39 chrome/test/functional/nacl_sdk.py, line 46 chrome/test/functional/nacl_sdk.py, line 52 chrome/test/functional/nacl_sdk.py, line 54 chrome/test/functional/nacl_sdk.py, line 56 chrome/test/functional/nacl_sdk.py, line 68 chrome/test/functional/nacl_sdk.py, line 81 chrome/test/functional/nacl_sdk.py, line 89 chrome/test/functional/nacl_sdk.py, line 97 chrome/test/functional/nacl_sdk.py, line 131 chrome/test/functional/nacl_sdk.py, line 147 chrome/test/functional/nacl_sdk.py, line 162 chrome/test/functional/nacl_sdk.py, line 164 chrome/test/functional/nacl_sdk.py, line 179 chrome/test/functional/nacl_sdk.py, line 181 chrome/test/functional/nacl_sdk.py, line 188 chrome/test/functional/nacl_sdk.py, line 190 chrome/test/functional/nacl_sdk.py, line 193 chrome/test/functional/nacl_sdk.py, line 197 chrome/test/functional/nacl_sdk.py, line 201 chrome/test/functional/nacl_sdk.py, line 209 chrome/test/functional/nacl_sdk.py, line 214 chrome/test/functional/nacl_sdk.py, line 220 chrome/test/functional/nacl_sdk.py, line 230 chrome/test/functional/nacl_sdk.py, line 237 chrome/test/functional/nacl_sdk.py, line 250 chrome/test/functional/nacl_sdk.py, line 271 chrome/test/functional/nacl_sdk.py, line 276 chrome/test/functional/nacl_sdk.py, line 282 chrome/test/functional/nacl_sdk.py, line 284 chrome/test/functional/nacl_sdk.py, line 298 chrome/test/functional/nacl_sdk.py, line 325 chrome/test/functional/nacl_sdk.py, line 335 chrome/test/functional/nacl_sdk.py, line 342 chrome/test/functional/nacl_sdk.py, line 344 chrome/test/functional/nacl_sdk.py, line 356 chrome/test/functional/nacl_sdk.py, line 382 chrome/test/functional/nacl_sdk.py, line 385 chrome/test/functional/nacl_sdk.py, line 391 chrome/test/functional/nacl_sdk.py, line 403 chrome/test/functional/nacl_sdk.py, line 412 chrome/test/functional/nacl_sdk.py, line 438 chrome/test/functional/nacl_sdk.py, line 445 chrome/test/functional/nacl_sdk.py, line 453 chrome/test/functional/nacl_sdk.py, line 456 chrome/test/functional/nacl_sdk.py, line 469 chrome/test/functional/nacl_sdk.py, line 472 chrome/test/functional/nacl_sdk.py, line 481 chrome/test/functional/nacl_sdk.py, line 499 chrome/test/functional/nacl_sdk.py, line 512 chrome/test/functional/nacl_sdk.py, line 517 chrome/test/functional/nacl_sdk.py, line 530 chrome/test/functional/nacl_sdk.py, line 550 chrome/test/functional/nacl_sdk.py, line 555 chrome/test/functional/nacl_sdk.py, line 570 chrome/test/functional/nacl_sdk.py, line 575 chrome/test/functional/nacl_sdk.py, line 577 chrome/test/functional/nacl_sdk.py, line 592 chrome/test/functional/nacl_sdk.py, line 611 chrome/test/functional/nacl_sdk.py, line 628 chrome/test/functional/nacl_sdk.py, line 642 chrome/test/functional/nacl_sdk.py, line 654 chrome/test/functional/nacl_sdk.py, line 657 chrome/test/functional/nacl_sdk.py, line 663 chrome/test/functional/nacl_sdk.py, line 672 chrome/test/functional/nacl_sdk.py, line 683 chrome/test/functional/nacl_sdk.py, line 692 chrome/test/functional/nacl_sdk.py, line 703 chrome/test/functional/nacl_sdk.py, line 710 chrome/test/functional/nacl_sdk.py, line 721 chrome/test/functional/nacl_sdk.py, line 728 chrome/test/functional/nacl_sdk.py, line 735 chrome/test/functional/nacl_sdk.py, line 745 chrome/test/functional/nacl_sdk.py, line 749 chrome/test/functional/nacl_sdk.py, line 754 chrome/test/functional/nacl_sdk.py, line 761 chrome/test/functional/nacl_sdk.py, line 770 chrome/test/functional/nacl_sdk.py, line 777 chrome/test/functional/nacl_sdk.py, line 799 chrome/test/functional/nacl_sdk.py, line 805 chrome/test/functional/nacl_sdk.py, line 813 chrome/test/functional/nacl_sdk.py, line 826 chrome/test/functional/nacl_sdk.py, line 833 chrome/test/functional/nacl_sdk.py, line 842 chrome/test/functional/nacl_sdk.py, line 845 chrome/test/functional/nacl_sdk.py, line 853 chrome/test/functional/nacl_sdk.py, line 867 chrome/test/functional/nacl_sdk.py, line 868 chrome/test/functional/nacl_sdk.py, line 876 chrome/test/functional/nacl_sdk.py, line 880 chrome/test/functional/nacl_sdk.py, line 892 chrome/test/functional/nacl_sdk.py, line 900 chrome/test/functional/nacl_sdk.py, line 904 chrome/test/functional/nacl_sdk.py, line 910 chrome/test/functional/nacl_sdk.py, line 926 chrome/test/functional/nacl_sdk.py, line 929 chrome/test/functional/nacl_sdk.py, line 937 chrome/test/functional/nacl_sdk.py, line 940 chrome/test/functional/nacl_sdk.py, line 952 chrome/test/functional/nacl_sdk.py, line 962 chrome/test/functional/nacl_sdk.py, line 975 chrome/test/functional/nacl_sdk.py, line 989 chrome/test/functional/nacl_sdk.py, line 992 chrome/test/functional/nacl_sdk.py, line 1006 chrome/test/functional/nacl_sdk.py, line 1012 chrome/test/functional/nacl_sdk.py, line 1014 chrome/test/functional/nacl_sdk.py, line 1018 chrome/test/functional/nacl_sdk.py, line 1021 chrome/test/functional/nacl_sdk.py, line 1036 chrome/test/functional/nacl_sdk.py, line 1048 chrome/test/functional/nacl_sdk.py, line 1052 chrome/test/functional/nacl_sdk.py, line 1061 chrome/test/functional/nacl_sdk.py, line 1063 chrome/test/functional/nacl_sdk.py, line 1071 chrome/test/functional/nacl_sdk.py, line 1074 chrome/test/functional/nacl_sdk.py, line 1079 chrome/test/functional/nacl_sdk.py, line 1084 chrome/test/functional/nacl_sdk.py, line 1091 ***************
Presubmit check for 7541046-21001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found line ending with white spaces in: *************** chrome/test/functional/nacl_sdk.py, line 162 chrome/test/functional/nacl_sdk.py, line 188 chrome/test/functional/nacl_sdk.py, line 190 chrome/test/functional/nacl_sdk.py, line 220 chrome/test/functional/nacl_sdk.py, line 230 chrome/test/functional/nacl_sdk.py, line 271 chrome/test/functional/nacl_sdk.py, line 335 chrome/test/functional/nacl_sdk.py, line 342 chrome/test/functional/nacl_sdk.py, line 356 chrome/test/functional/nacl_sdk.py, line 382 chrome/test/functional/nacl_sdk.py, line 445 chrome/test/functional/nacl_sdk.py, line 512 chrome/test/functional/nacl_sdk.py, line 570 chrome/test/functional/nacl_sdk.py, line 628 chrome/test/functional/nacl_sdk.py, line 663 chrome/test/functional/nacl_sdk.py, line 868 chrome/test/functional/nacl_sdk.py, line 976 *************** Presubmit checks took 1.3s to calculate.
Presubmit check for 7541046-21001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found line ending with white spaces in: *************** chrome/test/functional/nacl_sdk.py, line 162 chrome/test/functional/nacl_sdk.py, line 188 chrome/test/functional/nacl_sdk.py, line 190 chrome/test/functional/nacl_sdk.py, line 220 chrome/test/functional/nacl_sdk.py, line 230 chrome/test/functional/nacl_sdk.py, line 271 chrome/test/functional/nacl_sdk.py, line 335 chrome/test/functional/nacl_sdk.py, line 342 chrome/test/functional/nacl_sdk.py, line 356 chrome/test/functional/nacl_sdk.py, line 382 chrome/test/functional/nacl_sdk.py, line 445 chrome/test/functional/nacl_sdk.py, line 512 chrome/test/functional/nacl_sdk.py, line 570 chrome/test/functional/nacl_sdk.py, line 628 chrome/test/functional/nacl_sdk.py, line 663 chrome/test/functional/nacl_sdk.py, line 868 chrome/test/functional/nacl_sdk.py, line 976 *************** Presubmit checks took 1.0s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Presubmit check for 7541046-21002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found line ending with white spaces in: *************** chrome/test/functional/nacl_sdk.py, line 512 chrome/test/functional/nacl_sdk.py, line 570 chrome/test/functional/nacl_sdk.py, line 628 chrome/test/functional/nacl_sdk.py, line 663 chrome/test/functional/nacl_sdk.py, line 868 chrome/test/functional/nacl_sdk.py, line 976 *************** Presubmit checks took 1.1s to calculate.
Change committed as 96230 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
