Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10024031:
Nacl sdk automated test(method-testNaClSDK) of new installation process of NaCl sdk. (Closed)
|
Created:
8 years, 8 months ago by Pooja Nihalani Modified:
8 years, 7 months ago Visibility:
Public. |
DescriptionNacl sdk automated test(method-testNaClSDK) of new installation process of NaCl sdk.
Patch Set 1 : #
Total comments: 43
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 17
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 25 (0 generated)
Hello Anantha, Can you please review my changes and let me know if you want me to make any other changes? Thanks, Pooja
Removed all unused functions and executed stability of test on each OS.
On 2012/04/20 16:55:24, pnihalani wrote: > Removed all unused functions and executed stability of test on each OS. Pair review again today at 3 ok? We can get this checked in today! In the meanwhile, in the description of the CL, can you add as many comments as possible about the changes from the original nacl_sdk.py. For example: - Added test for Load Progress Example. - Removed screen capture test. - Updated sdk extract method.
3pm today looks good. Will add detailed description. Thanks, Pooja On 2012/04/20 18:44:42, chrisphan wrote: > On 2012/04/20 16:55:24, pnihalani wrote: > > Removed all unused functions and executed stability of test on each OS. > > Pair review again today at 3 ok? We can get this checked in today! > > In the meanwhile, in the description of the CL, can you add as many comments as > possible about the changes from the original nacl_sdk.py. > For example: > - Added test for Load Progress Example. > - Removed screen capture test. > - Updated sdk extract method.
Thanks for working on this. Remember to address all my comments or simply mark them as 'Done'. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:27: import time Move this import up along with other python lib imports. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:73: msg = 'Cannot open URL: %s' % Remove spaces for keyword arguments. Repeat everywhere else. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:85: ] Are these the only items you need to verify exist? https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:103: def _Verifyinstall(self): _VerifyInstall https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:108: self._extracted_sdk_path+'\\nacl_sdk', 'naclsdk.bat') Remove string cat in path join. Do same for everywhere else. source_file = os.path.join( self._extracted_sdk_path, 'nacl_sdk', 'naclsdk.bat') https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:112: process_chmod = subprocess.call(['chmod', '-R', '755', You can remove 'process_chmod' var if ur not using it. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:120: def _Verifyupdate(self): _VerifyUpdate https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:131: updated_output, error = subprocess.Popen( Comment here what you're doing. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:140: lambda x:x>='pepper_18', self._updated_pepper_versions) Add a TODO here to resolve this constant value. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:145: examples_path = os.path.join(self._extracted_sdk_path + '\\nacl_sdk\\' + Line up params. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:150: self._latest_updated_pepper_versions[0], Line up params. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:164: timeout = 150, retry_sleep = 1, expect_retval = True) Do you need timeout to be this long? Default is 30s. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:186: os.chdir(self._extracted_sdk_path+'.\..') Use os.path.join. Same for everywhere else. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:201: success = self.WaitUntil(_RemoveFile, retry_sleep=20, 'retry_sleep' here is too long. The default 'timeout' is 30s, so here you're retrying once, and waiting extra 20s of doing nothing. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:256: 'web_socket':self._VerifyWebSocketExample, Add two more spaces, line 249-256. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:462: self.ExecuteJavascript(js_code, tab_index) Not asserting anything, you can remove line 457-462 https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:495: re.search(r"NO|YES|42|MAYBE NOT|DEFINITELY|" Single quote https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:517: re.search(r"(loadstart).+(progress:).+(load).+(loadend).+(lastError:)", single quote https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:699: zip.extractall(xpath) use inline: zip.extractall(self._extracted_sdk_path) https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:757: def ExtraChromeFlags(self): Do they want you to use Chrome flags to enable NaCL? If so you don't need '_EnableNaClPlugin' method below. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:789: self.assertNotEquals(self.FindInPage('--enable-nacl')['match_count'], 0, Are you sure this is tab 0?
https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:27: import time On 2012/04/23 02:03:37, chrisphan wrote: > Move this import up along with other python lib imports. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:73: msg = 'Cannot open URL: %s' % On 2012/04/23 02:03:37, chrisphan wrote: > Remove spaces for keyword arguments. Repeat everywhere else. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:85: ] Yes, these are the only items in that folder which are present and are verified below. On 2012/04/23 02:03:37, chrisphan wrote: > Are these the only items you need to verify exist? https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:103: def _Verifyinstall(self): On 2012/04/23 02:03:37, chrisphan wrote: > _VerifyInstall Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:108: self._extracted_sdk_path+'\\nacl_sdk', 'naclsdk.bat') On 2012/04/23 02:03:37, chrisphan wrote: > Remove string cat in path join. Do same for everywhere else. > source_file = os.path.join( > self._extracted_sdk_path, 'nacl_sdk', 'naclsdk.bat') Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:112: process_chmod = subprocess.call(['chmod', '-R', '755', On 2012/04/23 02:03:37, chrisphan wrote: > You can remove 'process_chmod' var if ur not using it. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:120: def _Verifyupdate(self): On 2012/04/23 02:03:37, chrisphan wrote: > _VerifyUpdate Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:131: updated_output, error = subprocess.Popen( On 2012/04/23 02:03:37, chrisphan wrote: > Comment here what you're doing. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:140: lambda x:x>='pepper_18', self._updated_pepper_versions) On 2012/04/23 02:03:37, chrisphan wrote: > Add a TODO here to resolve this constant value. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:145: examples_path = os.path.join(self._extracted_sdk_path + '\\nacl_sdk\\' + On 2012/04/23 02:03:37, chrisphan wrote: > Line up params. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:150: self._latest_updated_pepper_versions[0], On 2012/04/23 02:03:37, chrisphan wrote: > Line up params. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:164: timeout = 150, retry_sleep = 1, expect_retval = True) Yes, it other wise crashes on windows.. On 2012/04/23 02:03:37, chrisphan wrote: > Do you need timeout to be this long? Default is 30s. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:186: os.chdir(self._extracted_sdk_path+'.\..') Do you want me to use os.path.join and then do a chdir ? I am not sure if "folder1 .\.." is possible using os.path.join Have used os.path.join where ever necessary above. Will check if there is way I can use it here. On 2012/04/23 02:03:37, chrisphan wrote: > Use os.path.join. Same for everywhere else. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:201: success = self.WaitUntil(_RemoveFile, retry_sleep=20, On 2012/04/23 02:03:37, chrisphan wrote: > 'retry_sleep' here is too long. The default 'timeout' is 30s, so here you're > retrying once, and waiting extra 20s of doing nothing. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:256: 'web_socket':self._VerifyWebSocketExample, On 2012/04/23 02:03:37, chrisphan wrote: > Add two more spaces, line 249-256. Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:462: self.ExecuteJavascript(js_code, tab_index) On 2012/04/23 02:03:37, chrisphan wrote: > Not asserting anything, you can remove line 457-462 Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:495: re.search(r"NO|YES|42|MAYBE NOT|DEFINITELY|" On 2012/04/23 02:03:37, chrisphan wrote: > Single quote Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:517: re.search(r"(loadstart).+(progress:).+(load).+(loadend).+(lastError:)", On 2012/04/23 02:03:37, chrisphan wrote: > single quote Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:699: zip.extractall(xpath) On 2012/04/23 02:03:37, chrisphan wrote: > use inline: > zip.extractall(self._extracted_sdk_path) Done. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:757: def ExtraChromeFlags(self): Yes they want chrome flags to be enabled. On 2012/04/23 02:03:37, chrisphan wrote: > Do they want you to use Chrome flags to enable NaCL? If so you don't need > '_EnableNaClPlugin' method below. https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:789: self.assertNotEquals(self.FindInPage('--enable-nacl')['match_count'], 0, Self.assertNotEquals.. is tabbed as in to fall in _EnableNaClPlugin method. On 2012/04/23 02:03:37, chrisphan wrote: > Are you sure this is tab 0?
https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk.py File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/3001/functional/nacl_sdk... functional/nacl_sdk.py:186: os.chdir(self._extracted_sdk_path+'.\..') On 2012/04/23 21:23:54, pnihalani wrote: > Do you want me to use os.path.join and then do a chdir ? I am not sure if > "folder1 .\.." is possible using os.path.join > > Have used os.path.join where ever necessary above. Will check if there is way I > can use it here. > On 2012/04/23 02:03:37, chrisphan wrote: > > Use os.path.join. Same for everywhere else. > Yes, that is possible. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:23: import time Imports should be alphabetically ordered. import re import shutil import subprocess import sys import time https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:102: self.fail(msg = 'NaCl SDK does not support this OS.') Remove spaces. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:115: self.fail(msg = 'NaCl SDK does not support this OS.') Remove spaces. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:192: self.fail(msg = 'NaCl SDK does not support this OS.') Remove spaces. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:198: shutil.rmtree(self._extracted_sdk_path, ignore_errors = True) Remove spaces. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:255: 'multithreaded_input_events': self._VerifyMultithreadedInputEventsExample, Longer than 80 chars. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:591: msg = 'Crash dump %s found' % dmp_file) Remove spaces https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:682: self.fail(msg = 'Cannot open %s.' % file_path) Spaces https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:762: def _EnableNaClPlugin(self): You can remove this method since you're enabling NaCl with flag.
https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:23: import time On 2012/04/24 01:22:05, chrisphan wrote: > Imports should be alphabetically ordered. > > import re > import shutil > import subprocess > import sys > import time Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:102: self.fail(msg = 'NaCl SDK does not support this OS.') On 2012/04/24 01:22:05, chrisphan wrote: > Remove spaces. Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:115: self.fail(msg = 'NaCl SDK does not support this OS.') On 2012/04/24 01:22:05, chrisphan wrote: > Remove spaces. Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:192: self.fail(msg = 'NaCl SDK does not support this OS.') On 2012/04/24 01:22:05, chrisphan wrote: > Remove spaces. Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:198: shutil.rmtree(self._extracted_sdk_path, ignore_errors = True) On 2012/04/24 01:22:05, chrisphan wrote: > Remove spaces. Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:255: 'multithreaded_input_events': self._VerifyMultithreadedInputEventsExample, On 2012/04/24 01:22:05, chrisphan wrote: > Longer than 80 chars. Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:591: msg = 'Crash dump %s found' % dmp_file) On 2012/04/24 01:22:05, chrisphan wrote: > Remove spaces Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:682: self.fail(msg = 'Cannot open %s.' % file_path) On 2012/04/24 01:22:05, chrisphan wrote: > Spaces Done. https://chromiumcodereview.appspot.com/10024031/diff/10001/functional/nacl_sd... functional/nacl_sdk.py:762: def _EnableNaClPlugin(self): DOne. On 2012/04/24 01:22:05, chrisphan wrote: > You can remove this method since you're enabling NaCl with flag.
lgtm with nits https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:132: updated_output, error=subprocess.Popen( Spaces here. updated_output, error = subprocess.Popen https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:134: stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate() One less space. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:177: 'http://localhost:5103/multithreaded_input_events/mt_input_events.html', More than 80 characters. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:199: Spaces in empty line. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:202: Spaces in empty line. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:325: Remove spaces in this empty line. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:506: tab_index))),timeout=10, expect_retval=True) Missing a space here.
https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:132: updated_output, error=subprocess.Popen( On 2012/04/24 18:34:00, chrisphan wrote: > Spaces here. > updated_output, error = subprocess.Popen Done. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:134: stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate() On 2012/04/24 18:34:00, chrisphan wrote: > One less space. Done. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:177: 'http://localhost:5103/multithreaded_input_events/mt_input_events.html', On 2012/04/24 18:34:00, chrisphan wrote: > More than 80 characters. Done. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:199: On 2012/04/24 18:34:00, chrisphan wrote: > Spaces in empty line. Done. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:202: On 2012/04/24 18:34:00, chrisphan wrote: > Spaces in empty line. Done. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:325: On 2012/04/24 18:34:00, chrisphan wrote: > Remove spaces in this empty line. Done. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:506: tab_index))),timeout=10, expect_retval=True) On 2012/04/24 18:34:00, chrisphan wrote: > Missing a space here. Done.
One more thing. https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:141: lambda x:x>='pepper_18', self._updated_pepper_versions) You're comparing string here. This will add 'pepper_17' in the list as well. To get the pepper version # only: self._updated_pepper_versions.extend( re.findall('Updating bundle pepper_([0-9]{2})', updated_output)) The rest you can just do a for loop and convert the number to int.
https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:141: lambda x:x>='pepper_18', self._updated_pepper_versions) I wanted to get string not just the version number. This string also refers to a folder in nacl_sdk. On 2012/04/24 19:19:10, chrisphan wrote: > You're comparing string here. This will add 'pepper_17' in the list as well. > To get the pepper version # only: > self._updated_pepper_versions.extend( > re.findall('Updating bundle pepper_([0-9]{2})', updated_output)) > > The rest you can just do a for loop and convert the number to int.
https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... File functional/nacl_sdk.py (right): https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... functional/nacl_sdk.py:141: lambda x:x>='pepper_18', self._updated_pepper_versions) On 2012/04/24 19:24:38, pnihalani wrote: > I wanted to get string not just the version number. This string also refers to a > folder in nacl_sdk. > > On 2012/04/24 19:19:10, chrisphan wrote: > > You're comparing string here. This will add 'pepper_17' in the list as well. > > To get the pepper version # only: > > self._updated_pepper_versions.extend( > > re.findall('Updating bundle pepper_([0-9]{2})', updated_output)) > > > > The rest you can just do a for loop and convert the number to int. > After you compare int, then you can append it to 'pepper_', because the logic here won't work with comparing string.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2012/04/24 19:57:17, chrisphan wrote: > https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... > File functional/nacl_sdk.py (right): > > https://chromiumcodereview.appspot.com/10024031/diff/15001/functional/nacl_sd... > functional/nacl_sdk.py:141: lambda x:x>='pepper_18', > self._updated_pepper_versions) > On 2012/04/24 19:24:38, pnihalani wrote: > > I wanted to get string not just the version number. This string also refers to > a > > folder in nacl_sdk. > > > > On 2012/04/24 19:19:10, chrisphan wrote: > > > You're comparing string here. This will add 'pepper_17' in the list as > well. > > > To get the pepper version # only: > > > self._updated_pepper_versions.extend( > > > re.findall('Updating bundle pepper_([0-9]{2})', updated_output)) > > > > > > The rest you can just do a for loop and convert the number to int. > > > > After you compare int, then you can append it to 'pepper_', because the logic > here won't work with comparing string. Ok looks good.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Please let me know if I am missing on making any changes you mentioned yesterday.
LGTM My concern is that a test could get out of sync with the tester however simple the test may be. My other concern is that since Chrome is always ahead of the SDK, any time someone adds a new example and new test for that example, the SDK containing it will not be ready, so the tree will go red until that SDK is built and AND gets pushed out which is currently days from when it's written. For now we could make sure to keep this file the MIN between all available tests in Chrome and all available tests in the SDK. Or we would need to let the SDK provide the list of which files may be tested.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pnihalani@chromium.org/10024031/25003
Presubmit check for 10024031-25003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/test/functional/nacl_sdk.py, line 179, 104 chars Found line ending with white spaces in: *************** chrome/test/functional/nacl_sdk.py, line 57 chrome/test/functional/nacl_sdk.py, line 108 chrome/test/functional/nacl_sdk.py, line 114 chrome/test/functional/nacl_sdk.py, line 116 chrome/test/functional/nacl_sdk.py, line 117 chrome/test/functional/nacl_sdk.py, line 123 chrome/test/functional/nacl_sdk.py, line 130 chrome/test/functional/nacl_sdk.py, line 154 chrome/test/functional/nacl_sdk.py, line 158 chrome/test/functional/nacl_sdk.py, line 167 chrome/test/functional/nacl_sdk.py, line 176 chrome/test/functional/nacl_sdk.py, line 178 chrome/test/functional/nacl_sdk.py, line 188 chrome/test/functional/nacl_sdk.py, line 271 chrome/test/functional/nacl_sdk.py, line 327 chrome/test/functional/nacl_sdk.py, line 345 chrome/test/functional/nacl_sdk.py, line 558 chrome/test/functional/nacl_sdk.py, line 674 chrome/test/functional/nacl_sdk.py, line 758 *************** Presubmit checks took 1.2s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pnihalani@chromium.org/10024031/30003
Presubmit check for 10024031-30003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit Warnings ** Found line ending with white spaces in: *************** chrome/test/functional/nacl_sdk.py, line 179 *************** Presubmit checks took 1.4s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pnihalani@chromium.org/10024031/28004
Change committed as 135483
|
