Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9585034:
Add extension policy tests (Closed)
|
Created:
8 years, 9 months ago by krisr Modified:
8 years, 9 months ago CC:
chromium-reviews, scottzcr Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd extension policy tests
TEST=Ran it!
BUG=116642
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126670
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 11 (0 generated)
https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... File chrome/test/functional/policy.py (right): https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:199: _screen_capture_crx_id = 'cpngackimfmofbokmjmljamhdncknpmg' constants should be capital. https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:201: def _buildCRXPath(self, crx_file_name): capitalize first letter. Also other places. https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:202: """ Returns the complete path to a crx_file in the data directory no space after quotation. https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:222: install_successful = False this variable has the opposite meaning of what it does https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:224: self.assertEqual(e[0], u'Extension could not be installed', do you need unicode for this to work? https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:268: self.assertFalse(self._checkForExtensionByID(PolicyTest._good_crx_id), Don't you already this verification in attemptExtension... https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:271: # Check adblock is installed Period at the end. https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:277: # TODO: Remove this when crosbug.com/27227 is fixed. This should be fixed now, give a try.
Also make your description more descriptive.
PTAL On Tue, Mar 6, 2012 at 2:55 PM, <frankf@chromium.org> wrote: > Also make your description more descriptive. > > https://chromiumcodereview.**appspot.com/9585034/<https://chromiumcodereview.... >
Please run gpylint. https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... File chrome/test/functional/policy.py (right): https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:202: """Returns the complete path to a crx_file in the data directory period. https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:205: crx_file_name: The file name of the extension period everywhere! https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:217: crx_file_name: The file name of the extension Raises section https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:223: except JSONInterfaceError, e: Please rebase your code. You have to do pyauto_errors.JSON... https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:233: """ Returns if an extension is installed No space before Returns. Also in other places. https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:250: if self._CheckForExtensionByID(PolicyTest.SCREEN_CAPTURE_CRX_ID): don't need the PolicyTest. prefix https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:271: ext_id = self.InstallExtension(self._BuildCRXPath('adblock.crx')) Please factor this to AttemptExtensionInstallThatShouldPass https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functio... chrome/test/functional/policy.py:319: # Give the system 30 seconds to go get this extension. can you explain the logic for waiting here.
PTAL On Tue, Mar 13, 2012 at 5:25 PM, <frankf@chromium.org> wrote: > Please run gpylint. > > This was a lot of noise. > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py> > File chrome/test/functional/policy.**py (right): > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode202<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode202> > > chrome/test/functional/policy.**py:202: """Returns the complete path to a > crx_file in the data directory > period. > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode205<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode205> > chrome/test/functional/policy.**py:205: crx_file_name: The file name of > the extension > period everywhere! > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode217<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode217> > chrome/test/functional/policy.**py:217: crx_file_name: The file name of > the extension > Raises section > Huh what? > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode223<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode223> > chrome/test/functional/policy.**py:223: except JSONInterfaceError, e: > Please rebase your code. You have to do pyauto_errors.JSON... > > You sure? See line 39 > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode233<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode233> > chrome/test/functional/policy.**py:233: """ Returns if an extension is > installed > No space before Returns. Also in other places. > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode250<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode250> > chrome/test/functional/policy.**py:250: if > self._CheckForExtensionByID(**PolicyTest.SCREEN_CAPTURE_CRX_**ID): > don't need the PolicyTest. prefix > Yeah I do or I get: ====================================================================== ERROR: policy.PolicyTest.testExtensionInstallWithGlobalBlacklistAndWhitelistedExtension: "Verify whitelisted extension is installed when all are blacklisted." ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/local/autotest/deps/chrome_test/test_src/chrome/test/functional/policy.py", line 303, in testExtensionInstallWithGlobalBlacklistAndWhitelistedExtension 'ExtensionInstallWhitelist': [ADBLOCK_CRX_ID] NameError: global name 'ADBLOCK_CRX_ID' is not defined ---------------------------------------------------------------------- > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode271<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode271> > chrome/test/functional/policy.**py:271: ext_id = > self.InstallExtension(self._**BuildCRXPath('adblock.crx')) > Please factor this to AttemptExtensionInstallThatSho**uldPass > > https://chromiumcodereview.**appspot.com/9585034/diff/** > 11001/chrome/test/functional/**policy.py#newcode319<https://chromiumcodereview.appspot.com/9585034/diff/11001/chrome/test/functional/policy.py#newcode319> > chrome/test/functional/policy.**py:319: # Give the system 30 seconds to go > get this extension. > can you explain the logic for waiting here. > > https://chromiumcodereview.**appspot.com/9585034/<https://chromiumcodereview.... >
https://chromiumcodereview.appspot.com/9585034/diff/17002/chrome/test/functio... File chrome/test/functional/policy.py (right): https://chromiumcodereview.appspot.com/9585034/diff/17002/chrome/test/functio... chrome/test/functional/policy.py:199: SCREEN_CAPTURE_CRX_ID = 'cpngackimfmofbokmjmljamhdncknpmg' _GOOD_CRX_ID https://chromiumcodereview.appspot.com/9585034/diff/17002/chrome/test/functio... chrome/test/functional/policy.py:261: if self._CheckForExtensionByID(PolicyTest.SCREEN_CAPTURE_CRX_ID): self._SCREEN_CAPTURE_CRX_ID
lgtm
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.
Other ideas... https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... File chrome/test/functional/policy.py (right): https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:329: Other test cases to cover: 1) Ext on Forcelist cannot be removed or disabled by user (compare to those only on whitelist, which can be removed/disabled). 2) Ext on Whitelist and Forcelist behaves the same as Forcelist only. 3) Ext on Forcelist and Blacklist behaves the same as on Forcelist only (force overrides black) 4) Ext on Forcelist only is removed from device when removed from Forcelist. 5) Ext on Forcelist and Whitelist is not removed from device when removed from Forcelist. https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/function... chrome/test/functional/policy.py:329: InstallExtension() method says that Ext must not be already installed. Should maybe try: 1) Installing Ext on both Whitelist and Forcelist. Should fail. 2) Installing Ext on Whitelist twice.
On Wed, Mar 14, 2012 at 8:00 AM, <scunningham@google.com> wrote: > Other ideas... > > > > https://chromiumcodereview.**appspot.com/9585034/diff/5001/** > chrome/test/functional/policy.**py<https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/functional/policy.py> > File chrome/test/functional/policy.**py (right): > > https://chromiumcodereview.**appspot.com/9585034/diff/5001/** > chrome/test/functional/policy.**py#newcode329<https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/functional/policy.py#newcode329> > chrome/test/functional/policy.**py:329: > Other test cases to cover: > 1) Ext on Forcelist cannot be removed or disabled by user (compare to > those only on whitelist, which can be removed/disabled). > 2) Ext on Whitelist and Forcelist behaves the same as Forcelist only. > 3) Ext on Forcelist and Blacklist behaves the same as on Forcelist only > (force overrides black) > 4) Ext on Forcelist only is removed from device when removed from > Forcelist. > 5) Ext on Forcelist and Whitelist is not removed from device when > removed from Forcelist. > > https://chromiumcodereview.**appspot.com/9585034/diff/5001/** > chrome/test/functional/policy.**py#newcode329<https://chromiumcodereview.appspot.com/9585034/diff/5001/chrome/test/functional/policy.py#newcode329> > chrome/test/functional/policy.**py:329: > InstallExtension() method says that Ext must not be already installed. > Should maybe try: > 1) Installing Ext on both Whitelist and Forcelist. Should fail. > Forcelist is broken on all platforms, I tried last night. This needs to be fixed before I can write those tests. > 2) Installing Ext on Whitelist twice. > > Chrome prohibits installing an extension twice, the pyauto hook throws an exception when you attempt this. > https://chromiumcodereview.**appspot.com/9585034/<https://chromiumcodereview.... > |
