Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10310047:
Fix testNaClSDK (Closed)
|
Created:
8 years, 7 months ago by Pooja Nihalani Modified:
8 years, 7 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionNacl sdk Nacl sdk automated test(method-testNaClSDK) of new installation process of NaCl sdk.
-Changed list of files being checked for NaCl sdk installer.
-Changed Download and unziping of nacl_sdk folder on all the platforms(Used Zipfile module).
-Changed installation of NaCl SDK.
-Removed Hello world examples from test case.
-Added new examples like Input Events, Dynamic Library(Magic * ball), Load progress, Multi-threaded input events, Web socket
Code Cleanup:
-Removed unused methods and pre-defined global variables.
-Removed pre-release gallery test.test(method-testNaClSDK) of new installation process of NaCl sdk.
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137538
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : #
Total comments: 22
Patch Set 4 : #
Total comments: 31
Patch Set 5 : #
Total comments: 21
Patch Set 6 : #
Total comments: 31
Patch Set 7 : #
Total comments: 8
Patch Set 8 : #Patch Set 9 : #
Total comments: 22
Patch Set 10 : #Patch Set 11 : #Messages
Total messages: 31 (0 generated)
I have created a new CL and made changes to PYAUTO_TESTS. Please let me know if I have to make any changes. Thanks, Pooja
On 2012/05/07 19:24:29, Pooja Nihalani wrote: LGTM This is a resubmit from Issue: http://codereview.chromium.org/10024031/
On 2012/05/07 19:50:15, chrisphan wrote: > On 2012/05/07 19:24:29, Pooja Nihalani wrote: > LGTM > > This is a resubmit from Issue: > http://codereview.chromium.org/10024031/ Don't commit this yet. Wait until you commit PYAUTO_TESTS.
Hi Nirnimesh, I have committed these changes before and these changes were reverted because there were few old test enabled in PYAUTO_TESTS. I have disabled the non existing test in nacl_sdk.py. I wanted to commit same changes as in CL http://codereview.chromium.org/10024031. Also, I tried disabling all the nacl_sdk reference in this CL but was not able to commit it; Do you know what these commit errors mean? http://codereview.chromium.org/10388014 Please review the code and let me know if I should make any other changes. Thanks, Pooja
I can't quite make head or tail of what you're trying to do in this CL. The CL description isn't helpful either. http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:112: subprocess.call(['chmod', '-R', '755', self._extracted_sdk_path]) use os.chmod http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:115: output, error = subprocess.Popen([source_file, 'list'], output/error are unused http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:120: """Verify update of NACL sdk""" I can't tell what you're doing here. Where is the verification? http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:140: lambda x:x>='pepper_18', self._updated_pepper_versions) need space around arithmetic operands and after : http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:154: proc = subprocess.Popen(['make RUN'], shell=True) this is plain ugly. make the first arg a list of strings, and remove shell=True. And this works on linux only. What about other platforms? http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:155: success = self.WaitUntil( |success| unused? http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:157: timeout=150, retry_sleep=1, expect_retval=True) Does the default timeout not suffice? http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:162: 'input_events': 'http://localhost:5103/input_events/input_events.html', 80+ chars not permitted http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:179: % (self._latest_updated_pepper_versions[0], indent by 4 more spaces http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:754: return ['--flag-switches-begin', Do not override. Only append to the parent's list. http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:754: return ['--flag-switches-begin', Why the --flag-switches-begin/end? Just put your flags. Period
http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:112: subprocess.call(['chmod', '-R', '755', self._extracted_sdk_path]) Need to do it recursively. Can't do with os.chmod without writing an extra function. Do you want me to do that? On 2012/05/08 17:35:39, Nirnimesh wrote: > use os.chmod http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:115: output, error = subprocess.Popen([source_file, 'list'], On 2012/05/08 17:35:39, Nirnimesh wrote: > output/error are unused Done. http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:120: """Verify update of NACL sdk""" On 2012/05/08 17:35:39, Nirnimesh wrote: > I can't tell what you're doing here. Where is the verification? Done. http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:140: lambda x:x>='pepper_18', self._updated_pepper_versions) On 2012/05/08 17:35:39, Nirnimesh wrote: > need space around arithmetic operands > and after : Done. http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:154: proc = subprocess.Popen(['make RUN'], shell=True) It only works as one string "make RUN". It works on win, linux, mac as 'make' is from within the sdk shell=True is required for it to work on windows. On 2012/05/08 17:35:39, Nirnimesh wrote: > this is plain ugly. make the first arg a list of strings, and remove shell=True. > And this works on linux only. What about other platforms? http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:155: success = self.WaitUntil( On 2012/05/08 17:35:39, Nirnimesh wrote: > |success| unused? Done. http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:157: timeout=150, retry_sleep=1, expect_retval=True) Default timeout is not enough. Need to wait little longer. On 2012/05/08 17:35:39, Nirnimesh wrote: > Does the default timeout not suffice? http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:162: 'input_events': 'http://localhost:5103/input_events/input_events.html', On 2012/05/08 17:35:39, Nirnimesh wrote: > 80+ chars not permitted Done. http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:179: % (self._latest_updated_pepper_versions[0], On 2012/05/08 17:35:39, Nirnimesh wrote: > indent by 4 more spaces Done.
http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:112: subprocess.call(['chmod', '-R', '755', self._extracted_sdk_path]) On 2012/05/08 22:33:03, Pooja Nihalani wrote: > Need to do it recursively. Can't do with os.chmod without writing an extra > function. Do you want me to do that? > > On 2012/05/08 17:35:39, Nirnimesh wrote: > > use os.chmod > Would this work on win?
http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/12001/functional/nacl_sdk.py#new... functional/nacl_sdk.py:112: subprocess.call(['chmod', '-R', '755', self._extracted_sdk_path]) This is only for linux and mac,the check is on line # 109 On 2012/05/08 22:37:44, Nirnimesh wrote: > On 2012/05/08 22:33:03, Pooja Nihalani wrote: > > Need to do it recursively. Can't do with os.chmod without writing an extra > > function. Do you want me to do that? > > > > On 2012/05/08 17:35:39, Nirnimesh wrote: > > > use os.chmod > > > > Would this work on win?
http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:40: 'nativeclient-mirror/nacl/nacl_sdk/nacl_sdk.zip', nit: indent by 4 chars to the right (and while you're at it, do the same for line 38) http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:77: self.assertTrue(url_index > -1, replace this and the last line with: self.assertTrue(sdk_url in html, ...) http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:123: source_file = os.path.join(self._extracted_sdk_path, 'nacl_sdk', save these instead of computing again http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:127: 'naclsdk') 'naclsdk' is twice? http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:154: proc = subprocess.Popen(['make RUN'], shell=True) ['make', 'RUN'] doesn't work? http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:185: version_number = re.findall('pepper_([0-9]{2})',pepper_version) need a blank space after , http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:193: os.chdir(os.path.join(self._extracted_sdk_path,'..')) need a blank space after , Repeat everywhere else in this file Use os.pardir instead of '..' http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:202: expect_retval=False) this is redundant http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:291: success = self.WaitUntil( It's now possible to wait for dom changes without polling. Look at examples in apptest.py. You could just wait for the eventString value to become of the given pattern http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:310: # Check if 'eventString' has handled above mouse click. same comment as above http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:358: success = self.WaitUntil( ditto http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:380: success = self.WaitUntil( ditto http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:400: success = self.WaitUntil( ditto Repeat everywhere else http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:452: lambda: bool(re.search(r"send:",self.GetDOMValue( use ' instead of " remove r http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:755: return ['--enable-nacl', Do not override. Append this list to pyauto.PyUITest.ExtraChromeFlags(self)
http://codereview.chromium.org/10310047/diff/4011/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (left): http://codereview.chromium.org/10310047/diff/4011/functional/PYAUTO_TESTS#old... functional/PYAUTO_TESTS:591: do not remove line http://codereview.chromium.org/10310047/diff/4011/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/10310047/diff/4011/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:583: # No NaCl support on ChromeOS. remove stray tab character
Hey Nirnimesh, I am afraid I accidentally deleted the last patchset (diff 4011) while uploading new patchsets and deleting the older sets keeping the recent one. I am sorry for that. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:40: 'nativeclient-mirror/nacl/nacl_sdk/nacl_sdk.zip', On 2012/05/08 23:16:57, Nirnimesh wrote: > nit: indent by 4 chars to the right > > (and while you're at it, do the same for line 38) Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:77: self.assertTrue(url_index > -1, On 2012/05/08 23:16:57, Nirnimesh wrote: > replace this and the last line with: > > self.assertTrue(sdk_url in html, ...) Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:123: source_file = os.path.join(self._extracted_sdk_path, 'nacl_sdk', These are path for different files naclsdk.bat and naclsdk. On 2012/05/08 23:16:57, Nirnimesh wrote: > save these instead of computing again http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:127: 'naclsdk') nacl_sdk is folder in side which there is a file naclsdk On 2012/05/08 23:16:57, Nirnimesh wrote: > 'naclsdk' is twice? http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:154: proc = subprocess.Popen(['make RUN'], shell=True) No On 2012/05/08 23:16:57, Nirnimesh wrote: > ['make', 'RUN'] doesn't work? http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:185: version_number = re.findall('pepper_([0-9]{2})',pepper_version) On 2012/05/08 23:16:57, Nirnimesh wrote: > need a blank space after , Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:193: os.chdir(os.path.join(self._extracted_sdk_path,'..')) On 2012/05/08 23:16:57, Nirnimesh wrote: > need a blank space after , > Repeat everywhere else in this file > > Use os.pardir instead of '..' Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:202: expect_retval=False) if I use self.assertFalse(_RemoveFile, msg='Cannot remove %s' % self._extracted_sdk_path) _RemoveFile always returns True which is not expected . So instead of calling _RemoveFile directly I have it called after a delay On 2012/05/08 23:16:57, Nirnimesh wrote: > this is redundant http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:291: success = self.WaitUntil( I have tried implementing event driven behavior as it is being done in apptest.py. I ran into specific errors. I see similar errors with apptest.py. I will stick with polling approach for now and once apptest.py is bit more stable, we can refactor. On 2012/05/08 23:16:57, Nirnimesh wrote: > It's now possible to wait for dom changes without polling. Look at examples in > apptest.py. > > You could just wait for the eventString value to become of the given pattern http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:310: # Check if 'eventString' has handled above mouse click. On 2012/05/08 23:16:57, Nirnimesh wrote: > same comment as above Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:358: success = self.WaitUntil( On 2012/05/08 23:16:57, Nirnimesh wrote: > ditto Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:380: success = self.WaitUntil( On 2012/05/08 23:16:57, Nirnimesh wrote: > ditto Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:400: success = self.WaitUntil( On 2012/05/08 23:16:57, Nirnimesh wrote: > ditto > > Repeat everywhere else Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:452: lambda: bool(re.search(r"send:",self.GetDOMValue( On 2012/05/08 23:16:57, Nirnimesh wrote: > use ' instead of " > > remove r Done. http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:755: return ['--enable-nacl', On 2012/05/08 23:16:57, Nirnimesh wrote: > Do not override. Append this list to pyauto.PyUITest.ExtraChromeFlags(self) Done.
http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/8004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:291: success = self.WaitUntil( On 2012/05/09 19:30:50, Pooja Nihalani wrote: > I have tried implementing event driven behavior as it is being done in > apptest.py. I ran into specific errors. I see similar errors with apptest.py. I > will stick with polling approach for now and once apptest.py is bit more stable, > we can refactor. That's unexpected. Could you send over the errors to craigdh@ please? http://codereview.chromium.org/10310047/diff/3003/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (left): http://codereview.chromium.org/10310047/diff/3003/functional/PYAUTO_TESTS#old... functional/PYAUTO_TESTS:43: 'chromoting_basic', Please re-sync. These should not be in your CL http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:193: os.chdir(os.path.join(self._extracted_sdk_path, os.pardir)) Do not chdir because it messes the pwd for other tests too. Why is it necessary? http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:349: js_code = """ This pattern seems to be repeated over and over again, with just the name of the element being different. Refactor out to a helper function. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:509: lambda: bool( typecasting to bool is not necessary http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:509: lambda: bool( Construct an inline nested function instead of a using multi-line lambda. It hampers readability. Repeat for all multi-line lambdas in this file http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:512: , tab_index))), timeout=10, expect_retval=True) expect_retval arg is redundant http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:755: self._EXTRA_CHROME_FLAGS = [ Why don't you use a local var -- extra_chrome_flags
http://codereview.chromium.org/10310047/diff/3003/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (left): http://codereview.chromium.org/10310047/diff/3003/functional/PYAUTO_TESTS#old... functional/PYAUTO_TESTS:43: 'chromoting_basic', On 2012/05/09 19:48:36, Nirnimesh wrote: > Please re-sync. These should not be in your CL Done. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:193: os.chdir(os.path.join(self._extracted_sdk_path, os.pardir)) pwd is inside the folder which is to be removed.Need to move out of the directory in order to delete it On 2012/05/09 19:48:36, Nirnimesh wrote: > Do not chdir because it messes the pwd for other tests too. Why is it necessary? http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:349: js_code = """ On 2012/05/09 19:48:36, Nirnimesh wrote: > This pattern seems to be repeated over and over again, with just the name of the > element being different. Refactor out to a helper function. Done. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:509: lambda: bool( On 2012/05/09 19:48:36, Nirnimesh wrote: > Construct an inline nested function instead of a using multi-line lambda. It > hampers readability. Repeat for all multi-line lambdas in this file Done. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:509: lambda: bool( It returns an object : <_sre.SRE_Match object at 0x01F71D78>, So I need to typecast it. On 2012/05/09 19:48:36, Nirnimesh wrote: > typecasting to bool is not necessary http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:512: , tab_index))), timeout=10, expect_retval=True) On 2012/05/09 19:48:36, Nirnimesh wrote: > expect_retval arg is redundant Done. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:755: self._EXTRA_CHROME_FLAGS = [ On 2012/05/09 19:48:36, Nirnimesh wrote: > Why don't you use a local var -- extra_chrome_flags Done.
The title of the CL should be something meaningful like: "Fix testNaClSDK" http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:153: os.chdir(examples_path) Reimplement this portion without needing to chdir() Will this help remove the other chdir? http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:169: 'pi_generator': 'http://localhost:5103/pi_generator/pi_generator.html', 80+ chars http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:509: lambda: bool( On 2012/05/09 23:31:41, Pooja Nihalani wrote: > It returns an object : <_sre.SRE_Match object at 0x01F71D78>, So I need to > typecast it. It's not necessary. SRE_Match object implicitly evaluates to True http://codereview.chromium.org/10310047/diff/14002/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (left): http://codereview.chromium.org/10310047/diff/14002/functional/PYAUTO_TESTS#ol... functional/PYAUTO_TESTS:43: 'chromoting_basic', You haven't synced still. These should not be there. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:115: stdout=subprocess.PIPE, weird indentation. align under [ on the previous line same for the next line http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:130: updated_output, error = subprocess.Popen( error is unused. Do: updated_output = subprocess.Popen(...).communicate()[0] http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:156: timeout=150, retry_sleep=1, expect_retval=True) exepct_retval arg is redundant. It checks for truth by default http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:168: 'pi_generator': 'http://localhost:5103/pi_generator/pi_generator.html', 80+ chars not allowed http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:183: """Check that chrome and pepper version mataches""" "Check that" -> Determine if matches -> match http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:277: element_id: Tab index integer that the example is on. tab index? I thought this was the dom element's id http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:281: var output = document.getElementById("%s").innerHTML; indent 281 - 287 by 2 more spaces to the right http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:294: var rightClick = document.createEvent('MouseEvents'); indent the js block by 2 more spaces to the right http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:300: ); align one space to the left http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:315: success = self.WaitUntil( move the WaitUntil portion into the helper too. Rename it as _VerifyElementPresent http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:342: tab_index).find('DidChangeView')+1), timeout=30,expect_retval=True) need space around + need space after , remove redundant expect_retval=True (I feel like I've pointed these out in my previous round of comments. Please enforce these throughout. I'm not pointing each and every occurrence) http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:365: js_code = self._CreateJS("eventString", "Received cancel") prefer ' over " Repeat throughout http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:411: var output = document.getElementById("log").value; use _CreateJS? http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:499: timeout=30, expect_retval='3.1') timeout=30 is the default I think. Why do you need to override? http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:513: tab_index),timeout=40, expect_retval='440') timeout=40 is quite close to the default timeout (~30 I think). Do you really need to override?
http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:153: os.chdir(examples_path) On 2012/05/10 01:05:16, Nirnimesh wrote: > Reimplement this portion without needing to chdir() > Will this help remove the other chdir? Done. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:169: 'pi_generator': 'http://localhost:5103/pi_generator/pi_generator.html', On 2012/05/10 01:05:16, Nirnimesh wrote: > 80+ chars Done. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:193: os.chdir(os.path.join(self._extracted_sdk_path, os.pardir)) On 2012/05/09 23:31:41, Pooja Nihalani wrote: > pwd is inside the folder which is to be removed.Need to move out of the > directory in order to delete it > > On 2012/05/09 19:48:36, Nirnimesh wrote: > > Do not chdir because it messes the pwd for other tests too. Why is it > necessary? > Done. http://codereview.chromium.org/10310047/diff/3003/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:509: lambda: bool( On 2012/05/10 01:05:16, Nirnimesh wrote: > On 2012/05/09 23:31:41, Pooja Nihalani wrote: > > It returns an object : <_sre.SRE_Match object at 0x01F71D78>, So I need to > > typecast it. > > It's not necessary. SRE_Match object implicitly evaluates to True Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:115: stdout=subprocess.PIPE, On 2012/05/10 01:05:16, Nirnimesh wrote: > weird indentation. > align under [ on the previous line > same for the next line Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:130: updated_output, error = subprocess.Popen( On 2012/05/10 01:05:16, Nirnimesh wrote: > error is unused. Do: > > updated_output = subprocess.Popen(...).communicate()[0] Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:156: timeout=150, retry_sleep=1, expect_retval=True) On 2012/05/10 01:05:16, Nirnimesh wrote: > exepct_retval arg is redundant. It checks for truth by default Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:168: 'pi_generator': 'http://localhost:5103/pi_generator/pi_generator.html', On 2012/05/10 01:05:16, Nirnimesh wrote: > 80+ chars not allowed Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:183: """Check that chrome and pepper version mataches""" On 2012/05/10 01:05:16, Nirnimesh wrote: > "Check that" -> Determine if > matches -> match Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:277: element_id: Tab index integer that the example is on. On 2012/05/10 01:05:16, Nirnimesh wrote: > tab index? I thought this was the dom element's id Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:281: var output = document.getElementById("%s").innerHTML; On 2012/05/10 01:05:16, Nirnimesh wrote: > indent 281 - 287 by 2 more spaces to the right Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:294: var rightClick = document.createEvent('MouseEvents'); On 2012/05/10 01:05:16, Nirnimesh wrote: > indent the js block by 2 more spaces to the right Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:300: ); On 2012/05/10 01:05:16, Nirnimesh wrote: > align one space to the left Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:315: success = self.WaitUntil( On 2012/05/10 01:05:16, Nirnimesh wrote: > move the WaitUntil portion into the helper too. > Rename it as _VerifyElementPresent Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:342: tab_index).find('DidChangeView')+1), timeout=30,expect_retval=True) On 2012/05/10 01:05:16, Nirnimesh wrote: > need space around + > need space after , > remove redundant expect_retval=True > > (I feel like I've pointed these out in my previous round of comments. Please > enforce these throughout. I'm not pointing each and every occurrence) Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:365: js_code = self._CreateJS("eventString", "Received cancel") On 2012/05/10 01:05:16, Nirnimesh wrote: > prefer ' over " > Repeat throughout Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:411: var output = document.getElementById("log").value; On 2012/05/10 01:05:16, Nirnimesh wrote: > use _CreateJS? Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:499: timeout=30, expect_retval='3.1') On 2012/05/10 01:05:16, Nirnimesh wrote: > timeout=30 is the default I think. Why do you need to override? Done. http://codereview.chromium.org/10310047/diff/14002/functional/nacl_sdk.py#new... functional/nacl_sdk.py:513: tab_index),timeout=40, expect_retval='440') It takes more than 30s. On 2012/05/10 01:05:16, Nirnimesh wrote: > timeout=40 is quite close to the default timeout (~30 I think). Do you really > need to override?
I still see unwanted bits in PYAUTO_TESTS. Please re-sync, and remove those before asking for review.
I took a quick look at the CL again. It's rife with several of the style issues I had pointed out in my previous round of comments. Please fix them ALL before asking for a review. http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:502: tab_index),timeout=40, expect_retval='440') why do you want to specify the timeout var? Isn't the default good enough? http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:519: 'document.getElementById("status_field").innerHTML', use ' instead of " http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:520: tab_index),timeout = 60, expect_retval='SUCCESS') need a space after , Remove space around = (Named args to functions should not have space around =) http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:710: ] indent by 2 spaces to the left
http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:502: tab_index),timeout=40, expect_retval='440') 30s is not enough,it takes longer. On 2012/05/11 09:13:32, Nirnimesh wrote: > why do you want to specify the timeout var? Isn't the default good enough? http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:519: 'document.getElementById("status_field").innerHTML', The double quote is inside single quote. Do you want me to change the " to \'? On 2012/05/11 09:13:32, Nirnimesh wrote: > use ' instead of " http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:520: tab_index),timeout = 60, expect_retval='SUCCESS') On 2012/05/11 09:13:32, Nirnimesh wrote: > need a space after , > > Remove space around = > (Named args to functions should not have space around =) Done. http://codereview.chromium.org/10310047/diff/5004/functional/nacl_sdk.py#newc... functional/nacl_sdk.py:710: ] On 2012/05/11 09:13:32, Nirnimesh wrote: > indent by 2 spaces to the left Done.
The PYAUTO_TESTS file still has other changes. Could you work with Chris to make sure you are able to sync and upload correctly?
A few more style nits, and 2 minor comments. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:167: 'http://localhost:5103/pi_generator/pi_generator.html', indent 4 spaces to the right http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:170: 'http://localhost:5103/websocket/websocket.html', indent by 4 spaces http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:180: self.GetBrowserInfo()['properties']['ChromeVersion'])) align under the previous line's 'self' http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:199: success = self.WaitUntil(_RemoveFile, retry_sleep=2, other than whitespace, anything else changed here? http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:251: self._VerifyMultithreadedInputEventsExample, indent by 4 chars http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:272: def _VerifyElementPresent(self, element_id, expected_value, tab_index, This function is not "verify'ing anything, since you don't have assert in it. I see that every usage of _verifyelementpresent() is followed by a call to assertTrue. The assertTrue should go inside here. Take msg as an arg. And do not return anything. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:274: """Determine if Dom element has the expected value. Dom -> dom http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:472: return re.search(r'(loadstart).+(progress:).+(load).+(loadend).+(lastError:)', 80+ chars not allowed http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:488: timeout=30, expect_retval='3.1') timeout=30 is the default anyway http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:502: tab_index),timeout=40, expect_retval='440') if you need only an additional 10 secs, it's likely that this test will fail if you run on another slightly slower machine. Better use one sufficiently large value for all timeouts. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:527: tab_index,timeout=60) need a space after ,
http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py File functional/nacl_sdk.py (right): http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:167: 'http://localhost:5103/pi_generator/pi_generator.html', On 2012/05/11 21:23:07, Nirnimesh wrote: > indent 4 spaces to the right Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:170: 'http://localhost:5103/websocket/websocket.html', On 2012/05/11 21:23:07, Nirnimesh wrote: > indent by 4 spaces Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:180: self.GetBrowserInfo()['properties']['ChromeVersion'])) On 2012/05/11 21:23:07, Nirnimesh wrote: > align under the previous line's 'self' Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:199: success = self.WaitUntil(_RemoveFile, retry_sleep=2, Nope On 2012/05/11 21:23:07, Nirnimesh wrote: > other than whitespace, anything else changed here? http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:251: self._VerifyMultithreadedInputEventsExample, On 2012/05/11 21:23:07, Nirnimesh wrote: > indent by 4 chars Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:272: def _VerifyElementPresent(self, element_id, expected_value, tab_index, On 2012/05/11 21:23:07, Nirnimesh wrote: > This function is not "verify'ing anything, since you don't have assert in it. > I see that every usage of _verifyelementpresent() is followed by a call to > assertTrue. The assertTrue should go inside here. Take msg as an arg. > And do not return anything. Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:274: """Determine if Dom element has the expected value. On 2012/05/11 21:23:07, Nirnimesh wrote: > Dom -> dom Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:472: return re.search(r'(loadstart).+(progress:).+(load).+(loadend).+(lastError:)', On 2012/05/11 21:23:07, Nirnimesh wrote: > 80+ chars not allowed Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:488: timeout=30, expect_retval='3.1') On 2012/05/11 21:23:07, Nirnimesh wrote: > timeout=30 is the default anyway Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:502: tab_index),timeout=40, expect_retval='440') On 2012/05/11 21:23:07, Nirnimesh wrote: > if you need only an additional 10 secs, it's likely that this test will fail if > you run on another slightly slower machine. Better use one sufficiently large > value for all timeouts. Done. http://codereview.chromium.org/10310047/diff/16003/functional/nacl_sdk.py#new... functional/nacl_sdk.py:527: tab_index,timeout=60) On 2012/05/11 21:23:07, Nirnimesh wrote: > need a space after , Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pnihalani@chromium.org/10310047/16004
Try job failure for 10310047-16004 (retry) (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Feel free to use NOTRY=true in the CL description.
Should this be committed?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pnihalani@chromium.org/10310047/16004
Change committed as 137538 |
