Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7633026:
Adding pyauto tests for the NaCl SDK. (Closed)
|
Created:
9 years, 4 months ago by chrisphan Modified:
9 years, 3 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=98757
Patch Set 1 #
Total comments: 39
Patch Set 2 : '' #
Total comments: 1
Patch Set 3 : '' #
Total comments: 8
Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #
Total comments: 10
Patch Set 6 : '' #
Total comments: 48
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 1
Patch Set 12 : '' #Patch Set 13 : '' #Messages
Total messages: 22 (0 generated)
Update for issue 7633026. Removed 7-Zip.
http://codereview.chromium.org/7633026/diff/1/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/7633026/diff/1/functional/PYAUTO_TESTS#newcode381 functional/PYAUTO_TESTS:381: 'nacl_sdk', Please move this up one line to keep the list in alphabetical order. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode73 functional/nacl_sdk.py:73: xml_str = response.read() No need to define this variable; just inline the code at the next line below. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode79 functional/nacl_sdk.py:79: == expected_md5_key: The python style guide says to not use backslash line continuation - an alternative is to use parens: if (self._GetXMLNodeData(con.getElementsByTagName('Key')[0].childNodes) == expected_md5_key): http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode94 functional/nacl_sdk.py:94: self.fail(msg='Cannot open %s.' % file_path) You may want to also retrieve and print out the exception message too: except IOError, e: self.fail(msg='Cannot open %s: %s' % (file_path, e)) http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode118 functional/nacl_sdk.py:118: for i in xrange(3): Why do we need to try this 3 times? It seems like a recipe for flakiness. If the download link is really not reliable, is it even a good idea to use it as part of a test? http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode177 functional/nacl_sdk.py:177: 'ncval_x86_64' nit: indent lines 174-177 by 2 more spaces each, to make it consistent with the lists above and below. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode395 functional/nacl_sdk.py:395: for i in xrange(6): Trying 6 times here to remove the files also seems like a recipe for flakiness. Under what conditions will the first try fail, but one of the subsequent tries succeeds? Is there any way to avoid having to try 6 times here? http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode460 functional/nacl_sdk.py:460: for _ in range(2): Why do we have to reload each example twice? http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode686 functional/nacl_sdk.py:686: for _ in xrange(tries): Is there any way to remove the fact that we have to try this multiple times? What is the reason we need to try multiple times here? http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode877 functional/nacl_sdk.py:877: if len(result) > 0: maybe just "if result:" http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode988 functional/nacl_sdk.py:988: if proc == None: 'proc' is an input argument to this function, and defaults to None. This check is basically saying that if the input argument is the default, then we fail the current test. That doesn't seem to make sense. If all we need to check is that the 'proc' input argument is specified, then why not just make it a required parameter, rather than an optional one? http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode1017 functional/nacl_sdk.py:1017: if len(files) == 0: maybe just this: "if not files:"
http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode73 functional/nacl_sdk.py:73: xml_str = response.read() On 2011/08/15 16:26:07, dennis_jeffrey wrote: > No need to define this variable; just inline the code at the next line below. Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode79 functional/nacl_sdk.py:79: == expected_md5_key: On 2011/08/15 16:26:07, dennis_jeffrey wrote: > The python style guide says to not use backslash line continuation - an > alternative is to use parens: > > if (self._GetXMLNodeData(con.getElementsByTagName('Key')[0].childNodes) > == expected_md5_key): Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode94 functional/nacl_sdk.py:94: self.fail(msg='Cannot open %s.' % file_path) On 2011/08/15 16:26:07, dennis_jeffrey wrote: > You may want to also retrieve and print out the exception message too: > > except IOError, e: > self.fail(msg='Cannot open %s: %s' % (file_path, e)) Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode118 functional/nacl_sdk.py:118: for i in xrange(3): I just follow the doc requirement for this one. It's the same with every tests that use link, sometimes you just don't get a response. But in my experience, it's more reliable than using pyauto.GetTabContents. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode177 functional/nacl_sdk.py:177: 'ncval_x86_64' On 2011/08/15 16:26:07, dennis_jeffrey wrote: > nit: indent lines 174-177 by 2 more spaces each, to make it consistent with the > lists above and below. Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode395 functional/nacl_sdk.py:395: for i in xrange(6): This is like the pyauto.WaitUntil. Waiting until the system closes the HTTP server which takes a second or two. This happens when someone interrupt the test before the HTTP server is closed. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode460 functional/nacl_sdk.py:460: for _ in range(2): That's what the doc wanted. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode686 functional/nacl_sdk.py:686: for _ in xrange(tries): The computer could be slow and it could take a bit longer to process the NaCl animation example. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode877 functional/nacl_sdk.py:877: if len(result) > 0: On 2011/08/15 16:26:07, dennis_jeffrey wrote: > maybe just "if result:" Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode988 functional/nacl_sdk.py:988: if proc == None: Proc is None when someone interrupts the test before the HTTP server is closed and since I don't have the pointer to the process that opened the HTTP server, I leave it as a fail. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode1017 functional/nacl_sdk.py:1017: if len(files) == 0: On 2011/08/15 16:26:07, dennis_jeffrey wrote: > maybe just this: "if not files:" Done.
http://codereview.chromium.org/7633026/diff/1/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/7633026/diff/1/functional/PYAUTO_TESTS#newcode381 functional/PYAUTO_TESTS:381: 'nacl_sdk', On 2011/08/15 16:26:07, dennis_jeffrey wrote: > Please move this up one line to keep the list in alphabetical order. Done.
A handful more comments. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode118 functional/nacl_sdk.py:118: for i in xrange(3): On 2011/08/15 17:00:51, chrisphan wrote: > I just follow the doc requirement for this one. It's the same with every tests > that use link, sometimes you just don't get a response. But in my experience, > it's more reliable than using pyauto.GetTabContents. Ok - I'm fine with this if it's indeed likely to "virtually always" access the link successfully within the first 3 tries (don't want to hang up this CL on that issue if this already pretty much works). But a better longer-term goal might be to figure out a way to write these tests in a way that doesn't depend on downloading things from live sites. This might be something to keep in mind for the future. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode395 functional/nacl_sdk.py:395: for i in xrange(6): On 2011/08/15 17:00:51, chrisphan wrote: > This is like the pyauto.WaitUntil. Waiting until the system closes the HTTP > server which takes a second or two. This happens when someone interrupt the > test before the HTTP server is closed. Hmm, in that case, could we use pyauto.WaitUntil instead? That might be more concise than trying to emulate its behavior yourself here. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode460 functional/nacl_sdk.py:460: for _ in range(2): On 2011/08/15 17:00:51, chrisphan wrote: > That's what the doc wanted. Hmm, do you know who wrote the doc to ask about this? I'm just curious as to why. In general, it's better to figure out why a doc says something if we don't know the reason. That's because docs can get outdated, or may even be incorrect. If that's too difficult to do now, but you think this is still worth investigating, maybe we could add a TODO to the code to look into this later. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode686 functional/nacl_sdk.py:686: for _ in xrange(tries): On 2011/08/15 17:00:51, chrisphan wrote: > The computer could be slow and it could take a bit longer to process the NaCl > animation example. If we can't easily avoid this, could we use pyauto's WaitUntil function here to make the code more concise? http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode988 functional/nacl_sdk.py:988: if proc == None: On 2011/08/15 17:00:51, chrisphan wrote: > Proc is None when someone interrupts the test before the HTTP server is closed > and since I don't have the pointer to the process that opened the HTTP server, I > leave it as a fail. Ok. How about just changing the condition to this then: 'if not proc:' And then modify the description of the 'proc' variable in the docstring above (line 974) to indicate the condition under which 'proc' is expected to be None. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode1017 functional/nacl_sdk.py:1017: if len(files) == 0: On 2011/08/15 17:00:51, chrisphan wrote: > On 2011/08/15 16:26:07, dennis_jeffrey wrote: > > maybe just this: "if not files:" > > Done. Oops, you need the "not", otherwise the logic is changed. I think it can be simplified to this since 'files' is a list: 'if not files:' http://codereview.chromium.org/7633026/diff/4003/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:876: result = fnmatch.filter(dirs, pattern) Since 'result' is a list, we can just say: 'if result:' This condition will be false if the list is empty, and true if the list is non-empty.
http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode395 functional/nacl_sdk.py:395: for i in xrange(6): On 2011/08/15 18:36:18, dennis_jeffrey wrote: > On 2011/08/15 17:00:51, chrisphan wrote: > > This is like the pyauto.WaitUntil. Waiting until the system closes the HTTP > > server which takes a second or two. This happens when someone interrupt the > > test before the HTTP server is closed. > > Hmm, in that case, could we use pyauto.WaitUntil instead? That might be more > concise than trying to emulate its behavior yourself here. Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode460 functional/nacl_sdk.py:460: for _ in range(2): I think the doc wanted to test performance/memory issue over time. The NaCl with animated examples hog a lot of cpu. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode686 functional/nacl_sdk.py:686: for _ in xrange(tries): On 2011/08/15 18:36:18, dennis_jeffrey wrote: > On 2011/08/15 17:00:51, chrisphan wrote: > > The computer could be slow and it could take a bit longer to process the NaCl > > animation example. > > If we can't easily avoid this, could we use pyauto's WaitUntil function here to > make the code more concise? Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode988 functional/nacl_sdk.py:988: if proc == None: On 2011/08/15 18:36:18, dennis_jeffrey wrote: > On 2011/08/15 17:00:51, chrisphan wrote: > > Proc is None when someone interrupts the test before the HTTP server is closed > > and since I don't have the pointer to the process that opened the HTTP server, > I > > leave it as a fail. > > Ok. How about just changing the condition to this then: > > 'if not proc:' > > And then modify the description of the 'proc' variable in the docstring above > (line 974) to indicate the condition under which 'proc' is expected to be None. Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode1017 functional/nacl_sdk.py:1017: if len(files) == 0: On 2011/08/15 18:36:18, dennis_jeffrey wrote: > On 2011/08/15 17:00:51, chrisphan wrote: > > On 2011/08/15 16:26:07, dennis_jeffrey wrote: > > > maybe just this: "if not files:" > > > > Done. > > Oops, you need the "not", otherwise the logic is changed. I think it can be > simplified to this since 'files' is a list: > > 'if not files:' Done.
A few more comments. Could you have Nirnimesh take a quick look too before you commit? (to ensure his issues from the previous CL have been addressed). Thanks! http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:399: success = self.WaitUntil(_RemoveFile, timeout=30, retry_sleep=0.25, The specified retry_sleep value here matches the default, so I think we can remove it from this argument list. http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:400: expect_retval=False) nit: indent this some more to line up underneath the first parameter from the previous line. http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:690: retry_sleep=retry_sleep, expect_retval=True) Could we just remove the "tries" and "retry_sleep" parameters from this function and hard-code the timeout value here? I think we can also just use the default value for "retry_sleep" from self.WaitUntil(). http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:691: return success Remove this line and just put return in front of the call to self.WaitUntil() above. http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1018: if not len(files): I think this has the same behavior. 'if not files'
Ok. http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:690: retry_sleep=retry_sleep, expect_retval=True) On 2011/08/15 22:27:45, dennis_jeffrey wrote: > Could we just remove the "tries" and "retry_sleep" parameters from this function > and hard-code the timeout value here? I think we can also just use the default > value for "retry_sleep" from self.WaitUntil(). Done. http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:691: return success On 2011/08/15 22:27:45, dennis_jeffrey wrote: > Remove this line and just put return in front of the call to self.WaitUntil() > above. Done. http://codereview.chromium.org/7633026/diff/7002/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1018: if not len(files): On 2011/08/15 22:27:45, dennis_jeffrey wrote: > I think this has the same behavior. > > 'if not files' Done.
Oops, I think there's a conflict with the PYAUTO_TESTS file. It seems you over-wrote the changes in that file (in the current CL) with the changes from your other pending CL. Could you fix that? http://codereview.chromium.org/7633026/diff/6005/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/6005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:678: retry_sleep: Sleep time in-between each try. Remove the 2 lines above. http://codereview.chromium.org/7633026/diff/6005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:690: retry_sleep=2, expect_retval=True) indent this line by 3 fewer spaces
http://codereview.chromium.org/7633026/diff/6005/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/6005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:678: retry_sleep: Sleep time in-between each try. On 2011/08/15 22:46:53, dennis_jeffrey wrote: > Remove the 2 lines above. Done. http://codereview.chromium.org/7633026/diff/6005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:690: retry_sleep=2, expect_retval=True) On 2011/08/15 22:46:53, dennis_jeffrey wrote: > indent this line by 3 fewer spaces Done.
http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode25 functional/nacl_sdk.py:25: need 2 blank lines at global scope http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode1063 functional/nacl_sdk.py:1063: def _GetTestSetting(self): Use EvalDataFrom() in PyUITest http://codereview.chromium.org/7633026/diff/4005/data/nacl_sdk/nacl_sdk_setting File data/nacl_sdk/nacl_sdk_setting (right): http://codereview.chromium.org/7633026/diff/4005/data/nacl_sdk/nacl_sdk_setti... data/nacl_sdk/nacl_sdk_setting:5: 'release_win_sdk_url': 'http://commondatastorage.googleapis.com/nativeclient-mirror/nacl/nacl_sdk/staging/naclsdk_win.exe', Fetching these packages from a fixed location has implications: - How do you deal with dev/beta/stable channels? http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1044: nacl_plugin = self.GetPluginsInfo().PluginForName('Chrome NaCl') lines 1044-1046 can be rewritten as: nacl_plugin = self.GetPluginsInfo().PluginForName('Chrome NaCl') or self.GetPluginsInfo().PluginForName('Native Client') http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1059: self.RestartBrowser(False) Does this work? Do you see --enable-nacl? I don't think the above approach of enabling nacl through about:flags would work. You should just pass --enable-nacl when launching chrome. See test_pyauto.py for examples http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1059: self.RestartBrowser(False) After this line, verify that --enable-nacl is passed to the browser.
http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode25 functional/nacl_sdk.py:25: On 2011/08/15 23:13:15, Nirnimesh wrote: > need 2 blank lines at global scope Done. http://codereview.chromium.org/7633026/diff/1/functional/nacl_sdk.py#newcode1063 functional/nacl_sdk.py:1063: def _GetTestSetting(self): On 2011/08/15 23:13:15, Nirnimesh wrote: > Use EvalDataFrom() in PyUITest Done.
Please respond to all the comments I made previously. http://codereview.chromium.org/7633026/diff/11001/data/nacl_sdk/nacl_sdk_setting File data/nacl_sdk/nacl_sdk_setting (right): http://codereview.chromium.org/7633026/diff/11001/data/nacl_sdk/nacl_sdk_sett... data/nacl_sdk/nacl_sdk_setting:1: # Settings for NaCl SDK testing. Can the contents of this file be kept in the nacl_sdk.py script please? Files that are expected to be shared across tests are kept in chrome/test/data. This one doesn't look like it will be shared. Keeping in the script will help keeping the test selfcontained. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:30: _extracted_sdk_path = os.path.join(_download_dir, 'extracted_nacl_sdk') Do not put downloaded files in the data dir. Keep them in some temp dir. tempfile.gettempdir() http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:34: self._RemoveDownloadedTestFile() and then this should not be necessary http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:82: if not expected_md5: self.assertTrue(expected_md5, msg='Cannot get ...') http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:86: try: Remove the try/except. Your default behavior does what a python exception would do anyway http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:88: md5.update(f.read()) md5.update(open(file_path).read()) and remove previous line http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:119: try: why the try block? http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:121: html = response.read() merge with previous line http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:124: time.sleep(1) why? http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:230: proc = subprocess.Popen( use subprocess.call() http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:290: proc = subprocess.Popen([scons_path], cwd=examples_path, shell=True) use subprocess.call() http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:294: if self._HasFile('*.nmf', ex_path): self.assertTrue(self._HasFile(...), msg='Failed..') Repeat this pattern everywhere else http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:298: proc = subprocess.Popen([scons_path], cwd=examples_path, subprocess.call() http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:299: stdout=subprocess.PIPE, shell=True) why shell=True? http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:330: shell=True, cwd=examples_path) why shell=True? http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:332: lines = stdout.splitlines() merge with previous line http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:334: for line in lines: lines 334-340: Replace with: self.assertTrue([x for x in lines if 'Check:' in x and 'passed' in x], msg='...') http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:345: self.fail(msg='Missing file: hello_world_x86_64.nexe.') use self.assertFalse http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:347: if not self._HasPathInTree('hello_world_x86_32.nexe', use self.assertFalse http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:400: success = self.WaitUntil(_RemoveFile, timeout=30, retry_sleep=2, remove the timeout arg. It defaults to ~25 secs anyway. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:400: success = self.WaitUntil(_RemoveFile, timeout=30, retry_sleep=2, why is waitUntil required for deleting the files? http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:408: if os.path.exists(sdk_path): use pyauto_utils.RemovePath(). Replace 405-412 with: [pyauto_utils.RemovePath(os.path.join(self._download_dir, x)) for x in ['naclsdk_win.exe', 'naclsdk_mac.tgz', 'naclsdk_linux.tgz'] ] http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:430: lines = stdout.splitlines() lines = (stdout + stderr).splitlines() http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:435: lines = stderr.splitlines() and get rid of this section
http://codereview.chromium.org/7633026/diff/4005/data/nacl_sdk/nacl_sdk_setting File data/nacl_sdk/nacl_sdk_setting (right): http://codereview.chromium.org/7633026/diff/4005/data/nacl_sdk/nacl_sdk_setti... data/nacl_sdk/nacl_sdk_setting:5: 'release_win_sdk_url': 'http://commondatastorage.googleapis.com/nativeclient-mirror/nacl/nacl_sdk/staging/naclsdk_win.exe', On 2011/08/15 23:13:15, Nirnimesh wrote: > Fetching these packages from a fixed location has implications: > > - How do you deal with dev/beta/stable channels? It looks like these links are updated with the latest sdk release. They come with minimum Chrome build requirement. They're aiming to have the sdk working on Chrome 10 and up. http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1059: self.RestartBrowser(False) On 2011/08/15 23:13:15, Nirnimesh wrote: > Does this work? Do you see --enable-nacl? > > I don't think the above approach of enabling nacl through about:flags would > work. > > You should just pass --enable-nacl when launching chrome. See test_pyauto.py for > examples The doc wanted to enable nacl through about:flags. http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1059: self.RestartBrowser(False) On 2011/08/15 23:13:15, Nirnimesh wrote: > After this line, verify that --enable-nacl is passed to the browser. Done. http://codereview.chromium.org/7633026/diff/11001/data/nacl_sdk/nacl_sdk_setting File data/nacl_sdk/nacl_sdk_setting (right): http://codereview.chromium.org/7633026/diff/11001/data/nacl_sdk/nacl_sdk_sett... data/nacl_sdk/nacl_sdk_setting:1: # Settings for NaCl SDK testing. On 2011/08/16 21:52:23, Nirnimesh wrote: > Can the contents of this file be kept in the nacl_sdk.py script please? > > Files that are expected to be shared across tests are kept in chrome/test/data. > This one doesn't look like it will be shared. Keeping in the script will help > keeping the test selfcontained. Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:30: _extracted_sdk_path = os.path.join(_download_dir, 'extracted_nacl_sdk') On 2011/08/16 21:52:23, Nirnimesh wrote: > Do not put downloaded files in the data dir. Keep them in some temp dir. > tempfile.gettempdir() Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:34: self._RemoveDownloadedTestFile() On 2011/08/16 21:52:23, Nirnimesh wrote: > and then this should not be necessary Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:82: if not expected_md5: On 2011/08/16 21:52:23, Nirnimesh wrote: > self.assertTrue(expected_md5, msg='Cannot get ...') Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:86: try: On 2011/08/16 21:52:23, Nirnimesh wrote: > Remove the try/except. Your default behavior does what a python exception would > do anyway Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:88: md5.update(f.read()) On 2011/08/16 21:52:23, Nirnimesh wrote: > md5.update(open(file_path).read()) and remove previous line Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:119: try: In-case the url is down or the request doesn't go through, give it another try. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:121: html = response.read() On 2011/08/16 21:52:23, Nirnimesh wrote: > merge with previous line Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:124: time.sleep(1) Giving it a break. I can remove it. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:230: proc = subprocess.Popen( On 2011/08/16 21:52:23, Nirnimesh wrote: > use subprocess.call() Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:290: proc = subprocess.Popen([scons_path], cwd=examples_path, shell=True) On 2011/08/16 21:52:23, Nirnimesh wrote: > use subprocess.call() Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:294: if self._HasFile('*.nmf', ex_path): On 2011/08/16 21:52:23, Nirnimesh wrote: > self.assertTrue(self._HasFile(...), msg='Failed..') > > Repeat this pattern everywhere else Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:298: proc = subprocess.Popen([scons_path], cwd=examples_path, Didn't work here. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:299: stdout=subprocess.PIPE, shell=True) Scons runs some python scripts. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:330: shell=True, cwd=examples_path) Scons runs some python scripts. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:332: lines = stdout.splitlines() On 2011/08/16 21:52:23, Nirnimesh wrote: > merge with previous line Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:334: for line in lines: It won't work here. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:345: self.fail(msg='Missing file: hello_world_x86_64.nexe.') On 2011/08/16 21:52:23, Nirnimesh wrote: > use self.assertFalse Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:347: if not self._HasPathInTree('hello_world_x86_32.nexe', On 2011/08/16 21:52:23, Nirnimesh wrote: > use self.assertFalse Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:400: success = self.WaitUntil(_RemoveFile, timeout=30, retry_sleep=2, On 2011/08/16 21:52:23, Nirnimesh wrote: > remove the timeout arg. It defaults to ~25 secs anyway. Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:400: success = self.WaitUntil(_RemoveFile, timeout=30, retry_sleep=2, The file may be locked up. So give some time for the process using it to stop. This happens when someone stops the script before the test is complete. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:408: if os.path.exists(sdk_path): On 2011/08/16 21:52:23, Nirnimesh wrote: > use pyauto_utils.RemovePath(). Replace 405-412 with: > > [pyauto_utils.RemovePath(os.path.join(self._download_dir, x)) for x in > ['naclsdk_win.exe', 'naclsdk_mac.tgz', 'naclsdk_linux.tgz'] ] Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:430: lines = stdout.splitlines() On 2011/08/16 21:52:23, Nirnimesh wrote: > lines = (stdout + stderr).splitlines() Done. http://codereview.chromium.org/7633026/diff/11001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:435: lines = stderr.splitlines() On 2011/08/16 21:52:23, Nirnimesh wrote: > and get rid of this section Done.
http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1059: self.RestartBrowser(False) Are you sure chrome really restarts with --enable-nacl after this line? I'm not sure if that's true.
http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1044: nacl_plugin = self.GetPluginsInfo().PluginForName('Chrome NaCl') On 2011/08/15 23:13:15, Nirnimesh wrote: > lines 1044-1046 can be rewritten as: > > nacl_plugin = self.GetPluginsInfo().PluginForName('Chrome NaCl') or > self.GetPluginsInfo().PluginForName('Native Client') Done. http://codereview.chromium.org/7633026/diff/4005/functional/nacl_sdk.py#newco... functional/nacl_sdk.py:1059: self.RestartBrowser(False) On 2011/08/17 23:58:45, Nirnimesh wrote: > Are you sure chrome really restarts with --enable-nacl after this line? I'm not > sure if that's true. Added a check in the latest copy.
Anything else I should look at here?
Code I commented on LGTM Just 1 nit. Please wait for Nirnimesh's approval before submitting. Thanks! http://codereview.chromium.org/7633026/diff/19001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/7633026/diff/19001/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:596: result_split = result.split(", ") use single quotes instead of double quotes.
LGTM. Sorry for the delay. If you don't get back a response on your CL in 1 day, please remind.
Use the commit checkbox to commit. Please watch the bots after committing.
Change committed as 98757 |
