Add tests for deploy_chrome and remote_access. [chromiumos/chromite : master]

[Ryan Cui (Code Review)]

Ryan Cui has uploaded a new change for review.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Add tests for deploy_chrome and remote_access.

Also adds in RunCommandMock object that can be reused by other test to
simulate RunCommand objects without using mox.

BUG=chromium-os:32575
TEST=ran unittests.

Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
---
M lib/cros_build_lib_unittest.py
M lib/cros_test_lib.py
A lib/remote_access_unittest.py
M scripts/deploy_chrome.py
A scripts/deploy_chrome_unittest.py
5 files changed, 364 insertions(+), 6 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/chromite refs/changes/54/29054/1
--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 3: I would prefer that you didn't submit this

Can you run pylint? pylint reports 37 warnings/errors for this CL :(

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

Gerrit-Reviewer: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>

[Brian Harring (Code Review)]

Brian Harring has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(10 inline comments)

....................................................
File lib/cros_build_lib_unittest.py
Line 38: self.backup = backup
this (passing the original RunCommand in) in parallel to GetContext I really, really don't like; the context manager should grab runcommand at the time of invocation.

Line 60: WriteLines(f, output.splitlines(), "echo %s\n")
drop the writelines function- it adds indirection/complexity here that a list comp or generator expression more clearly handles:

stdout = ['echo %s' % x for x in output.splitliines()]
stderr = ['echo %s >&2' % x for x in error.splitlines()]
f.write("\n".join(stdout + stderr))

Worth noting this could all just be osutils.WriteFile also.

Line 64: os.chmod(script, stat.S_IEXEC | stat.S_IREAD | stat.S_IWRITE)
os.chmod(script, 0700); no need for stat, and this is clearer to read (they're equivalent, but one has to look at the stat module to figure out if you're setting exec/read/write to all, or just user).

....................................................
File lib/cros_test_lib.py
Line 33: MATCH_ALL_CMDS = '**match_all_commands**'
this needs a lot more documentation- including potentially an example class w/in the docstring.

I say this since I'm looking at it, and have been for about 30-45s, and I don't exactly grok the intent. :)

Line 39: if type(cmd) is list:
isinstance(cmd, list); type() is generally wrong to use..

Line 47: def GetCmd(self, cmd):
def __call__ comes to mind...

Line 52: raise Exception("'%s' not mocked!" % cmd)
This probably should be an AssertionError since it's used in unittest context..

....................................................
File lib/remote_access_unittest.py
Line 41: # Run the real Remotesh with RunCommand mocked out.
RemoteSh.

Line 43: return super(RemoteAccessMock, self).RemoteSh(cmd, *args, **kwargs)
Don't use super here... invoke remote_access.RemoteAccess.RemoteSh directly; super generally would work, but it's reliant on inheritance- and generally is useful for notifications, not returns.

Line 45: def RemoteReboot(self, *args, **kwargs):
unused...?

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 3: (10 inline comments)

ptal.

....................................................
File lib/cros_build_lib_unittest.py
Line 38: self.backup = backup

Done

Line 60: WriteLines(f, output.splitlines(), "echo %s\n")

Done

Line 64: os.chmod(script, stat.S_IEXEC | stat.S_IREAD | stat.S_IWRITE)

Done

....................................................
File lib/cros_test_lib.py
Line 33: MATCH_ALL_CMDS = '**match_all_commands**'

Changed things up a bit. See if you like this better :).

Line 39: if type(cmd) is list:

Done

Line 47: def GetCmd(self, cmd):

Can you clarify this?

Line 52: raise Exception("'%s' not mocked!" % cmd)

Done

....................................................
File lib/remote_access_unittest.py
Line 41: # Run the real Remotesh with RunCommand mocked out.

Done

Line 43: return super(RemoteAccessMock, self).RemoteSh(cmd, *args, **kwargs)

Done

Line 45: def RemoteReboot(self, *args, **kwargs):

Done

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 4:

Will look at the pylint issues...

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 4: I would prefer that you didn't submit this

I'll wait for you to address the pylint issues before reviewing :)

************* Module lib.remote_access_unittest
W0212: 75:RemoteAccessTest.testCheckIfRebooted: Access to a protected member _CheckIfRebooted of a client class
W0212: 78:RemoteAccessTest.testCheckIfRebooted: Access to a protected member _CheckIfRebooted of a client class
W0212: 81:RemoteAccessTest.testCheckIfRebooted: Access to a protected member _CheckIfRebooted of a client class
W0212: 84:RemoteAccessTest.testCheckIfRebooted: Access to a protected member _CheckIfRebooted of a client class
************* Module scripts.deploy_chrome
W0613:241:check_gs_path: Unused argument 'opt'
W0613:241:check_gs_path: Unused argument 'option'
W0613:260:check_path: Unused argument 'opt'
W0613:260:check_path: Unused argument 'option'
W0613:310:_PostParseCheck: Unused argument 'args'
************* Module scripts.deploy_chrome_unittest
E1120: 33:InterfaceTest._mockParserError: No value passed for parameter 'msg' in function call
W0212: 40:InterfaceTest.testGsLocalPathUnSpecified: Access to a protected member _ParseCommandLine of a client class
W0212: 46:InterfaceTest.testGsPathSpecified: Access to a protected member _ParseCommandLine of a client class
W0212: 51:InterfaceTest.testLocalPathSpecified: Access to a protected member _ParseCommandLine of a client class
W0212: 58:InterfaceTest.testNoTarget: Access to a protected member _ParseCommandLine of a client class
W0212: 74:InterfaceTest.testGsPathCorrection: Access to a protected member _ParseCommandLine of a client class
E1101: 82:DeployChromeMock.__init__: Instance of 'RemoteAccess' has no 'MockCmd' member
E1101: 94:DeployChromeMock._DisableRootfsVerification: Instance of 'RemoteAccess' has no 'MockCmd' member
W0613: 96:DeployChromeMock.RemoteShHook: Unused argument 'error'
W0613: 96:DeployChromeMock.RemoteShHook: Unused argument 'kwargs'
W0613: 96:DeployChromeMock.RemoteShHook: Unused argument 'output'
W0613: 96:DeployChromeMock.RemoteShHook: Unused argument 'args'
W0212:106:TestContext: Access to a protected member _ParseCommandLine of a client class
W0212:118:TestPrepareTarget.testSuccess: Access to a protected member _PrepareTarget of a client class
W0212:127:TestPrepareTarget.testDisableRootfsVerificationFailure: Access to a protected member _PrepareTarget of a client class
W0212:136:TestPrepareTarget.testMountRwFailure: Access to a protected member _PrepareTarget of a client class
W0212:142:TestPrepareTarget.testMountRwSuccessFirstTime: Access to a protected member _PrepareTarget of a client class
W0212:155:TestCheckRootfs.MockOverride._CheckRootfsWriteable: Access to a protected member _CheckRootfsWriteable of a client class
W0212:161:TestCheckRootfs.testCheckRootfsWriteableFalse: Access to a protected member _CheckRootfsWriteable of a client class
W0212:167:TestCheckRootfs.testCheckRootfsWriteableTrue: Access to a protected member _CheckRootfsWriteable of a client class
W0212:175:TestUiJobStarted.testUiJobStartedFalse: Access to a protected member _CheckUiJobStarted of a client class
W0212:181:TestUiJobStarted.testCheckRootfsWriteableTrue: Access to a protected member _CheckUiJobStarted of a client class
Running ['/usr/local/chromeos/internal/src/scripts/sdk_lib/enter_chroot.sh', '--chroot', '/usr/local/chromeos/internal/chroot', '--cache_dir', '/usr/local/chromeos/internal/.cache', '--', 'sh', '-c', 'cd ~/trunk/chromite && echo lib/cros_build_lib_unittest.py lib/cros_test_lib.py lib/remote_access_unittest.py scripts/deploy_chrome.py scripts/deploy_chrome_unittest.py && pylint lib/cros_build_lib_unittest.py lib/cros_test_lib.py lib/remote_access_unittest.py scripts/deploy_chrome.py scripts/deploy_chrome_unittest.py'] failed with exit code 6

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 6:

pylint issues fixed.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 6:

ping.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 6: I would prefer that you didn't submit this

(22 inline comments)

....................................................
File lib/cros_build_lib_unittest.py
Line 50: stdout = ['echo %s\n' % x for x in output.splitlines()]
What if 'output' has shell characters in it? This seems like it'd break easily... even an apostrophe could break this...

One easy solution for this would be to store output in a temp file and just cat the temp file.

....................................................
File lib/cros_test_lib.py
Line 103: class MockableCmdMixin(object):
What's the point of this class if you aren't using any of the regex features? Everything so far just uses MATCH_ALL_CMDS ...

Line 123: self.mock_cmds[regex] = (returncode, output, error)
Why are you using a dictionary? Wouldn't it make more sense just to use a list of regex's?

The fact that you're silently dropping redundant calls to MockRegexCmd seems like a recipe for hiding bugs

Line 125: def GetCmd(self, cmd):
docstring?

Line 131: if matched_result:
if matched_result is not None ?

Line 132: raise AssertionError("cmd '%s' matches more than one mock!" % cmd)
%r instead of '%s' ?

Line 136: raise AssertionError("'%s' not mocked!" % cmd)
%r instead of '%s' ?

....................................................
File lib/remote_access_unittest.py
Line 7: """Unittests for chrome_set_ver."""
Docstring needs updating?

Line 37: if self.hook:
if self.hook is not None ?

Line 42: returncode, output, error)
indentation looks weird here...

Line 56: self.host.MockCmd('ls', returncode=0, output='some_files',
output and error could be moved into a constant.

optional: The test would also be more interesting if you added some entertainment. Brian's standard joke is:

output = 'monkeys'
error = 'fling poo'

... but feel free to be creative :) There are lots of other possible errors.

Line 69: self.host.MockCmd('ls', returncode=255)
We have a constant for this (SSH_ERROR_CODE) -- any reason not to use it?

Same below.

Line 92: unittest.main()
cros_test_lib.main(), no?

....................................................
File scripts/deploy_chrome.py
Line 91: if not self.host:
self.host is None ?

Line 251: value = value.rstrip('/ ')
Why are we stripping spaces?

....................................................
File scripts/deploy_chrome_unittest.py
Line 6: """Unittests for chrome_set_ver."""
chrome_set_ver is getting a lot of unit tests these days...

Line 24: class InterfaceTest(unittest.TestCase):
1. I think we're trying to avoid direct use of unittest and instead use cros_test_lib.TestCase. This ensures that logging, etc is setup.

2. Docstring ? Same for all classes below. Just a quick description of the type of tests that are in the class.

Line 32: argv = ['--to', 'monkey', '--gs-path', 'gs://foon']
Can we make monkey and gs://foon a constant as it's repeated many times in the test...

Line 67: super(DeployChromeMock, self).__init__(
Can we avoid use of super? See https://fuhm.net/super-harmful/

Previously I may have suggested use of super, but I didn't realize exactly how it worked :) Some rules:

- Subclasses must use super if their superclasses do

- Superclasses must use super if their subclasses do

Example: https://fuhm.net/super-harmful/example2-1.py returns C A B B when you'd expect it to return C A B

Line 83: def _KillProcsIfNeeded(self):
Why are you overriding this method with an empty method? This makes it difficult to test this method, no?

Line 105: class MockOverride(DeployChromeMock):
Could you add a comment on why you're overriding the _DisableRootfsVerification method?

Line 173: unittest.main()
cros_test_lib.main() ?

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 6: (20 inline comments)

Comments addressed. ptal. I'm also considering adding an attribute to mark a function as mocking out another function. i.e.,

class DeployChromeMock(DeployChrome):
@mock_function
_CheckRootfsWriteable(self):
...

And the @mock_function attribute will verify that the parent class actually has a function named _CheckRootfsWriteable. This ensures that we are not just thinking we are mocking something out.

Thoughts?

....................................................
File lib/cros_build_lib_unittest.py
Line 50: stdout = ['echo %s\n' % x for x in output.splitlines()]

Yup, will fix that.

....................................................
File lib/cros_test_lib.py
Line 103: class MockableCmdMixin(object):

The regex features aren't used outside of MATCH_ALL_CMDS yet. But MockCmd() is used in deploy_chrome_unittest.

Line 123: self.mock_cmds[regex] = (returncode, output, error)

Using a dictionary so mocks can be overridden - a useful feature. See the DeployChromeMock() class in deploy_chrome_unittest.py. To help with test debugging, I've added a debug message when mocks are overridden.

Line 125: def GetCmd(self, cmd):

Done

Line 132: raise AssertionError("cmd '%s' matches more than one mock!" % cmd)

Does it make a difference for regex?

....................................................
File lib/remote_access_unittest.py
Line 7: """Unittests for chrome_set_ver."""

Done

Line 37: if self.hook:
Done

Line 42: returncode, output, error)

Done

Line 56: self.host.MockCmd('ls', returncode=0, output='some_files',

Done

Line 69: self.host.MockCmd('ls', returncode=255)

Done

Line 92: unittest.main()
Done

....................................................
File scripts/deploy_chrome.py
Line 91: if not self.host:

Done

Line 251: value = value.rstrip('/ ')

Done

....................................................
File scripts/deploy_chrome_unittest.py
Line 6: """Unittests for chrome_set_ver."""

Done

Line 24: class InterfaceTest(unittest.TestCase):
Done

Line 32: argv = ['--to', 'monkey', '--gs-path', 'gs://foon']

Done

Line 67: super(DeployChromeMock, self).__init__(
Done

Line 83: def _KillProcsIfNeeded(self):
This class is the mock implementation of DeployChrome. Since there's nothing that depends on _KillProcsIfNeeded() actually doing anything, we can just no-op it here. If this method itself needs to be tested, we can add some extra mocking to this class to enable it to be run straight-up. If some tests rely on side-effects of this function, we an implement that here too.

Line 105: class MockOverride(DeployChromeMock):
Done

Line 173: unittest.main()
Done

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

Gerrit-Reviewer: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>

Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 7: I would prefer that you didn't submit this

(14 inline comments)

Getting closer...

....................................................
File lib/cros_build_lib_unittest.py
Line 40: backup = cros_build_lib.RunCommand
This should happen prior to the try -- technically, if this fails you don't want to assign cros_build_lib.RunCommand at all :)

Line 50: stdout = ['echo %s\n' % x for x in result.output.splitlines()]
From the previous patchset:

What if 'output' has shell characters in it? This seems like it'd break easily... even an apostrophe could break this...

One easy solution for this would be to store output in a temp file and just cat the temp file.

....................................................
File lib/cros_test_lib.py

Line 144: "cmd '%s' matches more than one mock:\n%s"
%r means that it prints the Python representation of the string, so that means that it'll automatically handle quoting and such if there are any apostrophes in the string. That's why I suggested using %r (without the quotes) as opposed to %s.

Line 147: raise AssertionError("'%s' not mocked!" % cmd)
same here, you can just use %r to get the python representation of the cmd

....................................................
File lib/osutils.py
Line 26: def Return01(boolean):
These functions are unnecessary. Booleans are already numbers so you can just inline them like this:

int(boolean)
int(not boolean)

....................................................
File lib/remote_access_unittest.py
Line 32: pass
No need for the pass statement if you've got a docstring. Brian has started suggesting that we adopt this style

Line 103: self.host.MockRegexCmd('.*%s.*' % remote_access.REBOOT_MARKER, returncode)
Suggest using re.escape around anything you substitute into a regex...

....................................................
File scripts/deploy_chrome_unittest.py
Line 53: self.assertRaises(SystemExit, _ParseCommandLine, argv)
For scripts where error output is normal, can you hide the output so that it doesn't junk up the output with errors? Having errors printed to the screen makes it look like the test is failing.

Line 57: sandbox_header = 'https://sandbox.google.com/storage'
This should be a constant and should be shared with the other places it is used (e.g. we literally use the same string in 2 other places. One of the other places doesn't use https currently but it probably could.) We should also split out the host so that lib/commandline.py can reuse this too.

Line 93: def _CheckRootfsWriteable(self):
As you suggested, @mock_function would make sense for this and everything else so as to make sure we're actually overriding something

Line 98: osutils.Return01(self.disable_ok))
Shorter:
int(not self.disable_ok)

Line 114: deploy = mock_class(options, tempdir, os.path.join(tempdir, 'staging'),
Any reason not to just yield this directly?

Line 171: with TestContext(self.MockOverride) as deploy:
This looks very similar to testCheckRootfsWriteableFalse -- can you write a helper function so that there's no copy/pasting ?

Line 191: deploy.host.MockCmd('status ui', output=output)
Same here, this looks like a copy/paste job from testUiJobStartedFalse

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 8:

Still some work left to do on this, but uploaded for dual purpose of preserving work and letting others take a look.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 7: (14 inline comments)

....................................................
File lib/cros_build_lib_unittest.py
Line 40: backup = cros_build_lib.RunCommand

Using mock library now, no longer valid.

Line 50: stdout = ['echo %s\n' % x for x in result.output.splitlines()]

Yup, will take care of this.

....................................................
File lib/cros_test_lib.py
Line 144: "cmd '%s' matches more than one mock:\n%s"

Done

Line 147: raise AssertionError("'%s' not mocked!" % cmd)

Done

....................................................
File lib/osutils.py
Line 26: def Return01(boolean):

My rationale was Return01(bool) is clearer than int(not boolean), which is perhaps debatable.

....................................................
File lib/remote_access_unittest.py
Line 32: pass

No longer valid.

Line 103: self.host.MockRegexCmd('.*%s.*' % remote_access.REBOOT_MARKER, returncode)

Done

....................................................
File scripts/deploy_chrome_unittest.py
Line 53: self.assertRaises(SystemExit, _ParseCommandLine, argv)

Added code to cros_test_lib to automatically set --buffer mode when running unittests.

Line 57: sandbox_header = 'https://sandbox.google.com/storage'

Seems like the implementation of the functionality is in flux (a-la https://gerrit.chromium.org/gerrit/#/c/37333/1/lib/gs.py). Will leave this in here until things are settled.

Line 93: def _CheckRootfsWriteable(self):
Using mock lib + cros_test_lib.MockObject.ATTR class variable to control this.

Line 98: osutils.Return01(self.disable_ok))
Done

Line 114: deploy = mock_class(options, tempdir, os.path.join(tempdir, 'staging'),

No longer valid.

Line 171: with TestContext(self.MockOverride) as deploy:

Done

Line 191: deploy.host.MockCmd('status ui', output=output)

Done

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 10:

feedback addressed, and the new mocking framework is done. ptal.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 10

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 10: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 10

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 10: Looks good to me, approved

(2 inline comments)

Nice! Please update docs to add 'mock' to the list of required files to install, and email chromeos-build@ before submitting

....................................................
File lib/cros_build_lib_unittest.py
Line 45: result.output, result.error)
indentation is off here

....................................................
File lib/cros_test_lib.py
Line 220: src_attr = '__target%s' % attr if attr.startswith('__') else attr
Double underscore is reserved for internal python magic -- suggest single underscore ?

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 10

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 11:

Changes in this patchset -

1. Removed automatically running tests in buffered mode, since it causes cgroups_unittest.py and cbuildbot_stages_unittest.py to hang, and cbuildbot_unittest.py to fail. I think it has something to do with interaction with mox, but just removing this for now.

2. Uses gs.py url canonicalization logic, and moves gs url tests to commandline_unittest.py.

3. Turn into standalone CL instead of depending on https://gerrit.chromium.org/gerrit/#/c/36020/ in preparation for check in.

4. Nits addressed.

Ptal.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 11

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Brian Harring (Code Review)]

Brian Harring has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 12: I would prefer that you didn't submit this

(16 inline comments)

I'd strongly like to see a standard mox version of this, to compare/contrast. I'm not convinced the alternate approach here is clearer/better.

....................................................
File lib/commandline_unittest.py
Line 44: """Test GS path normalization functionality."""
expand this docstring; specifically that your testing type=gs_path func implementation.

Also, _parseCommandLine and _CreateParser do not need to be separeate functions (and realistically this should be a method on GSPathTest)

....................................................
File lib/cros_test_lib.py
Line 119: if isinstance(cmd, list):
tuples...

Also, same issue that via this forcing stringification it can't tell the difference between multiple args and a single arg applies.

Line 134: will be simulated by the mock.
docstring here needs hefty expansion; this roughly is a stubout + defining an invocation + return, but that's /not/ obvious in reading this.

Line 169: # TODO: Remove this once we move to snake oil.
snakeoil is all one word. also chuck this sucker into cros_build_lib; wrapper and others could make use of it now.

Line 183: return sys.modules[name]
imo, drop this; the only reason to preserve it is for performance, and performance isn't an issue here.

Line 192: class MockObject(object):
The name here needs work; at first glance I would assume this is a tweaked mox.MockObject (since we do a bit of mox work in here), which it isn't....

Line 209: def Start(self):
__enter__

Line 223: def Stop(self):
__exit__

Line 230: self.patchers.pop(attr).stop()
this is dead/borked code.

Line 233: def GetMockContext(self):
This is unnecessary; the object itself could do context protocol.

....................................................
File lib/gs.py
Line 41: raise InvalidURLError('Url %s cannot be canonicalized.' % url)
any reasons this isn't just a ValueError?

....................................................
File scripts/deploy_chrome_unittest.py
Line 7: import mock
this import isn't particularly sorted yet...

Also it's unused.

Line 23: _REGULAR_TO = ['--to', 'monkey']
globals shouldn't be mutable (tuple).

Line 53: self.assertRaises(SystemExit, _ParseCommandLine, argv)

shouldn't you check the exit code?

Line 93: pass
raise NotImplementedError(self, '_KillProcsIfNeeded')

Line 149: self.mock.UnMockAttr('_CheckRootfsWriteable')
reason you're doing this, rather than just changing attrs?

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 12: (8 inline comments)

....................................................
File lib/cros_test_lib.py

Line 169: # TODO: Remove this once we move to snake oil.

Done

Line 183: return sys.modules[name]

Done

Line 192: class MockObject(object):
Done

Line 209: def Start(self):
Done

Line 223: def Stop(self):
Done

Line 230: self.patchers.pop(attr).stop()
Why is this dead/borked?

Line 233: def GetMockContext(self):
Done

....................................................
File scripts/deploy_chrome_unittest.py

Line 149: self.mock.UnMockAttr('_CheckRootfsWriteable')

It feels cleaner to do it this way - simple to unset a mock.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 12: (8 inline comments)

....................................................

File lib/commandline_unittest.py
Line 44: """Test GS path normalization functionality."""

Done

....................................................
File lib/cros_test_lib.py
Line 119: if isinstance(cmd, list):

Yup, noted. This can be improved upon in the future without drastically changing the interface. We can move to a more mox-like way of specifying comparators.

Line 134: will be simulated by the mock.

Done

....................................................
File lib/gs.py
Line 41: raise InvalidURLError('Url %s cannot be canonicalized.' % url)

Done

....................................................
File scripts/deploy_chrome_unittest.py
Line 7: import mock
It's used - see line 126. I will also be adding 'mock' to the chroot, and sending out an email to the team before I land this.

Line 23: _REGULAR_TO = ['--to', 'monkey']

Done

Line 53: self.assertRaises(SystemExit, _ParseCommandLine, argv)

Done

Line 93: pass
This function is actually stubbed out - it is actually called by deploy_chrome._PrepareTarget(), but just does nothing. Updated comments.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 13:

Comments addressed. Modified hook interface - hooks now are called with the full invocation of the mocked out command.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 13

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Brian Harring (Code Review)]

Brian Harring has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 14: (5 inline comments)

....................................................
File lib/commandline_unittest.py
Line 32: class GSPathTest(cros_test_lib.TestCase):
suggest checking '../blah/monkeys' being passed in, and that it complains appropriately.

....................................................
File lib/cros_build_lib_unittest.py
Line 27: # pylint: disable=W0212,W0233,R0904,E1101
Don't expand the global suppressions; suppress the individual offenses (else clarify why we need to disable 2 more checks globally).

....................................................
File lib/cros_test_lib.py
Line 119: if isinstance(cmd, (list, tuple)):
Suppose I wasn't clear; if you want to flatten like this, then invert the check- flatten everything that isn't what you expect.

Meaning:

if not isinstance(cmd, basestring):
cmd = ' '.join(cmd)

If we had that flatten function I mentioned, it would just be:

cmd = ' '.join(flatten(cmd))

Either way, the forms I've mentioned are more robust then just checking for list/tuple.

Line 212: def Stop(self):
our style guide not withstanding, Stop() vs stop() seems like a recipe to get bit in the ass; I've use a lowercase 'stop()' here instead of the usual capitalization.

....................................................
File scripts/deploy_chrome_unittest.py
Line 149: self.mock.UnMockAttr('_CheckRootfsWriteable')

you argued this was cleaner in patchset12; I think you meant 'easier'.

Either you can do this- which may get us into trouble if the base implementation ever changes so that you can't just unmock things mid test, or you could've just done thus:

ATTRS = tuple(x for x in DeployChromeTest.ATTRS if x != '_CheckRootfsWritable')

Don't really know what to say; I consider that cleaner than what you're advocating here where a setUp() runs, creates that patch, then the very next setUp() goes and tears down that patch...

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 14

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 14: (9 inline comments)

partial review

....................................................
File lib/commandline_unittest.py
Line 67: self.assertRaisesRegexp(SystemExit, '2', self._RunGSPathTestCase,
What's the '2' here?

....................................................
File lib/cros_test_lib.py
Line 118: def _ProcessCmd(self, cmd):
This is nice for comparing against a regex, but we want to be careful not to do this in general.

Line 132: Arguments:
Can you document these args?

Line 138: self.MockRegexCmd(re.escape(cmd), returncode=returncode, output=output,
Why is MockCmd implemented as a regex under the covers? I'd rather delete this function than implement it as a regex.

Here's some behavior that's strange in your implementation, as an example: MockCmd(['ls']) matches ['ls', 'foo'] -- why? I'd expect that 'ls' just matches literally: it should match 'ls'.

MockRegexCmd('ls') is a different matter -- I'd expect it to match any string that starts with ls since I'm explicitly passing in a regex.

Line 152: """Returns a MockResult object for the mocked command.
I don't understand -- what does this do? Can you add some comments to explain?

Line 156: arg_list: A list/tuple of args that
Looks like this sentence isn't finished...

....................................................
File lib/remote_access_unittest.py
Line 22: cros_test_lib.MockableCmdMixin):
The fact that the interface of MockableCmdMixin is being used to mock out RemoteSh calls is somewhat confusing, since it's basically limiting us to an implementation where RemoteSh only calls one command, that would be difficult to change without refactoring.

I think it'd be better in general if we just kept the details about the MockableCmdMixin private (e.g. private methods or using composition.) Composition seems clearer to me.

Line 39: rc_mock.MockRegexCmd(rc_mock.MATCH_ALL_CMDS, result.returncode,
Could this mock be a bit more specific?

Line 71: self.mock.MockCmd(self.TEST_CMD, returncode=RETURN_CODE, output=OUTPUT,
Can you make the meaning of this more clear somehow? It's not clear it's mocking out the remote sh command.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 14

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 15:

partial response to partial review. Ptal and comment on bigger picture stuff (api, etc.). Biggest changes are:

1. Renamed stuff in cros_test_lib.py
2. Pulled in some comparators from mox.py
3. Made MockableCmdMixin (renamed to MockedCallResults) more general, so it can be used to do more than just command mocking.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 15

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 15: (21 inline comments)

Generally, like the approach

....................................................
File lib/commandline_unittest.py
Line 9: import optparse
W0611: 9: Unused import optparse

Line 67: self.assertRaisesRegexp(SystemExit, '2', self._RunGSPathTestCase,

This is new in Python 2.7:
E1101: 67:GSPathTest.testInvalidPath: Instance of 'GSPathTest' has no 'assertRaisesRegexp' member

....................................................
File lib/cros_build_lib_unittest.py
Line 36: def __init__(self, tempdir):
Please add docstrings for everything with descriptions of their arguments etc.

Line 46: result.output, result.error)
indentation

....................................................
File lib/cros_test_lib.py
Line 12: import contextlib
Lots of warnings / errors:
W0232:118:Comparator: Class has no __init__ method
W0703:146:In.equals: Catch "Exception"
W0703:168:Regex.equals: Catch "Exception"
W0703:190:ListRegex.equals: Catch "Exception"
E1101:189:ListRegex.equals: Instance of 'ListRegex' has no 'ProcessRhs' member
W0232:194:IgnoreArg: Class has no __init__ method
W0613:215:MockedCallResults.AddResultForParams: Unused argument 'side_effect'
W0612:230:MockedCallResults.AddResultForParams.filter_fn: Unused variable 'filter_fn'
W0611: 12: Unused import contextlib

Line 111: def predicate_split(func, iterable):
This should probably be private, also, naming convention is strange -- Wouldn't we call this PredicateSplit or _PredicateSplit ?

Line 118: class Comparator:
This should inherit from object, right?

Line 121: def equals(self, rhs):
Isn't our naming standard to use capital 'e' for these?

Line 125: def __eq__(self, rhs):
Can we avoid using __eq__ / __ne__ at all here?

The following is confusing:

>>> x = cros_test_lib.In('foo')
>>> y = cros_test_lib.IgnoreArg()
>>> x == y
False
>>> y == x
True

Line 146: except Exception:
Can we check the type of rhs instead?

Line 168: except Exception:
Please don't catch exception :)

Line 172: s = '<regular expression \'%s\'' % self.regex.pattern
Can you use %r instead of \'%s\' ? :)

Line 213: return pv if isinstance(pv, tuple) else (pv,)
Why are we converting everything into tuples?

Line 224: Arguments:
Docs for the arguments? Please also document what types the arguments should be.

Line 234: dup, filtered = predicate_split(lambda p_r: p_r[0] == params,
This duplicate replacement doesn't work consistently for me, likely because of the difference between x==y and y==x.
>>> results.AddResultForParams(cros_test_lib.In('baz'), 'baz')
>>> results.AddResultForParams(['foo', 'bar'], 'baz')
>>> results.AddResultForParams(ignore, 'ignored')
>>> results.mocked_cmds
[(((<sequence or map containing 'baz'>,), None), 'baz'), (((<IgnoreArg>,), None), 'ignored')]

Unit tests?

Line 242: def LookupResult(self, pv, hook_pv, hook_kv, kv=None):
Docstring ?

Line 314: self.results = MockedCallResults(self.ATTRS[0])
Any reason why 'self.results' isn't private? Child classes might use this, but I'd expect that users of the class wouldn't directly muck with .results

....................................................
File lib/remote_access_unittest.py
Line 18: # pylint: disable=W0212,W0233,E1101
Are all of these still needed?

Line 70: output=output, error=error)
Indentation?

....................................................
File scripts/deploy_chrome.py
Line 12: import time
************* Module scripts.deploy_chrome
W0611: 10: Unused import optparse
W0611: 13: Unused import urlparse

....................................................
File scripts/deploy_chrome_unittest.py
Line 70: def hook(inst, *args, **kwargs):
More warnings:
W0613: 70:DeployChromeMock.MockMountCmd.hook: Unused argument 'args'
W0613: 70:DeployChromeMock.MockMountCmd.hook: Unused argument 'inst'
W0613: 70:DeployChromeMock.MockMountCmd.hook: Unused argument 'kwargs'

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 15

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 15: (19 inline comments)

....................................................
File lib/commandline_unittest.py
Line 9: import optparse

Done

Line 67: self.assertRaisesRegexp(SystemExit, '2', self._RunGSPathTestCase,

Done

....................................................
File lib/cros_build_lib_unittest.py
Line 36: def __init__(self, tempdir):

Done

Line 46: result.output, result.error)
Done

....................................................
File lib/cros_test_lib.py

Line 111: def predicate_split(func, iterable):

Moved to cros_build_lib.py

Line 118: class Comparator:
Done

Line 121: def equals(self, rhs):

Done

Line 125: def __eq__(self, rhs):

Done

Line 146: except Exception:
Catching TypeErorr now.

Line 168: except Exception:
Done

Line 172: s = '<regular expression \'%s\'' % self.regex.pattern

Done

Line 213: return pv if isinstance(pv, tuple) else (pv,)

Not doing this anymore.

Line 224: Arguments:
Done

Line 242: def LookupResult(self, pv, hook_pv, hook_kv, kv=None):

Done

Line 314: self.results = MockedCallResults(self.ATTRS[0])

Done

....................................................
File lib/remote_access_unittest.py
Line 18: # pylint: disable=W0212,W0233,E1101

Done

Line 70: output=output, error=error)
Done

....................................................
File scripts/deploy_chrome.py
Line 12: import time

Done

....................................................
File scripts/deploy_chrome_unittest.py
Line 70: def hook(inst, *args, **kwargs):

Done

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7
Gerrit-PatchSet: 15

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 18: I would prefer that you didn't submit this

(15 inline comments)

Almost there

....................................................
File lib/gs.py
Line 37: raise ValueError('Url %s cannot be canonicalized.' % url)
%r ?

....................................................
File lib/partial_mock.py
Line 28: def Equals(self, rhs):
Any reason why this isn't just overriding __eq__ ? This is literal equality so it makes sense.

Line 30: return type(self) == type(rhs) and self._Equals(rhs)
You can implement this more simply:

return type(self) == type(rhs) and self.__dict__ == rhs.__dict__

... and this saves you the trouble of implementing all the _Equals methods.

Line 52: return self._key == rhs._key
W0212: 52:In._Equals: Access to a protected member _key of a client class

Line 55: return '<sequence or map containing \'%s\'>' % str(self._key)
Why no %r? :)

Line 92: @staticmethod
Newline before this line?

Line 120: return sorted(d.iteritems(), key=lambda kv: kv[0])
key= is unnecessary assuming that there are no duplicates (which are impossible in a dict, right?)

Line 123: def _RecursiveCompare(lhs, rhs, equality=False):
This function seems complicated enough to be unit tested, no?

Line 136: if len(lhs) != len(rhs):
This would be shorter:

return (len(lhs) == len(rhs) and
all(_RecursiveCompare(i, j, equality) for i, j in zip(lhs, rhs)))

Line 139: if not _RecursiveCompare(i, j):
Doesn't equality need to be passed down here?

Line 143: return _RecursiveCompare(_SortedDict(lhs), _SortedDict(rhs))
Same here. Doesn't equality need to be passed down here?

Line 165: def __init__(self, name):
Docstring ?

Line 170: def AssertArgs(args, kwargs):
docstring ?

Line 328: cmd: The command string or list to record a result for.
Does this work with a list? It looks to me like it would only work with a string, given that we're wrapping everything as a tuple.

....................................................
File scripts/deploy_chrome_unittest.py
Line 37: check_attrs={'code': 2})
Indentation is off here.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7

Gerrit-PatchSet: 18

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 18: (13 inline comments)

....................................................
File lib/gs.py
Line 37: raise ValueError('Url %s cannot be canonicalized.' % url)

Done

....................................................
File lib/partial_mock.py
Line 28: def Equals(self, rhs):

Done

Line 30: return type(self) == type(rhs) and self._Equals(rhs)

Done

Line 52: return self._key == rhs._key

Done

Line 55: return '<sequence or map containing \'%s\'>' % str(self._key)

Done

Line 92: @staticmethod
Done

Line 120: return sorted(d.iteritems(), key=lambda kv: kv[0])

Done

Line 123: def _RecursiveCompare(lhs, rhs, equality=False):

Done

Line 136: if len(lhs) != len(rhs):

Done

Line 139: if not _RecursiveCompare(i, j):

Removed equality.

Line 143: return _RecursiveCompare(_SortedDict(lhs), _SortedDict(rhs))

Done

Line 170: def AssertArgs(args, kwargs):

Done

Line 328: cmd: The command string or list to record a result for.

Works with a list. We always pass a list or a tuple of arguments to AddResultForParams. This is true (and works) regardless if cmd is a list or a string.

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 18: (1 inline comment)

....................................................
File scripts/deploy_chrome_unittest.py
Line 37: check_attrs={'code': 2})

Done

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 20: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7

Gerrit-PatchSet: 20

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 20: Looks good to me, approved

(5 inline comments)

Final nits

....................................................
File lib/partial_mock.py
Line 162: assert isinstance(args, (list, tuple))
What if args is a basestring? In your current code, this is impossible, since args always wrapped in an enclosing tuple, but that won't be true after you address my suggestions.

Line 191: self.AssertArgs(args, kwargs)
This assert seems bogus, since args might be a comparator for example, so it won't be a list or a tuple in that case. Same for kwargs, which might be a comparator.

Currently this assert trips if kwargs is a comparator.

Line 330: (cmd,), result, kwargs=kwargs, side_effect=side_effect, strict=strict)
Why is cmd always wrapped in a tuple? Seems unnecessary and it also complicates the API for LookupResults since everybody has to add an unnecessary wrapping tuple.

....................................................
File lib/partial_mock_unittest.py
Line 124: def testKwargsNonStrictLookup(self):
Can we add some testing for using comparators on kwargs? Currently your CL is broken for this use case, so worth adding a test :)

....................................................
File lib/remote_access_unittest.py
Line 34: (cmd,), hook_args=(inst, cmd,) + args, hook_kwargs=kwargs)
Why is (cmd,) wrapped as a tuple? The invocation and mock is not wrapped as a tuple so this shouldn't be either...

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 20: (5 inline comments)

....................................................
File lib/partial_mock.py
Line 162: assert isinstance(args, (list, tuple))

As per talk with david, we are locking down args to tuples for now, and adding support for Comparator objects in a follow-up cl.

Line 191: self.AssertArgs(args, kwargs)
See above comment.

Line 330: (cmd,), result, kwargs=kwargs, side_effect=side_effect, strict=strict)

See above comment.

....................................................
File lib/partial_mock_unittest.py
Line 124: def testKwargsNonStrictLookup(self):

See comment in partial_mock.py

....................................................
File lib/remote_access_unittest.py
Line 34: (cmd,), hook_args=(inst, cmd,) + args, hook_kwargs=kwargs)

See comment in partial_mock.py

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 21: Verified; Looks good to me, approved; Ready

Inheriting LGTM. Adding CQ-DEPEND.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7

Gerrit-PatchSet: 21

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add tests for deploy_chrome and remote_access.
......................................................................

Patch Set 22: Verified; Looks good to me, approved; Ready

rebased. Trying again.

--
To view, visit https://gerrit.chromium.org/gerrit/29054
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41a32158d7a96d90b6bdfff21d195991e5b174b7

Gerrit-PatchSet: 22