Fix system_stub.Restore(), and add a unittest to catch the bug being fixed.

Fix system_stub.Restore(), and add a unittest to catch the bug being fixed.

BUG=466415
Committed: https://crrev.com/c12ba21289ad8a745e5526e547a78d92ab309ca8
Cr-Commit-Position: refs/heads/master@{#320385}

[aiolos (Not reviewing), 2015-03-12 00:15:21]

aiolos@chromium.org changed reviewers:
+ dpranke@chromium.org, dtu@chromium.org

[Dirk Pranke, 2015-03-12 00:26:29]

lgtm, with comments below.

https://codereview.chromium.org/999963002/diff/1/tools/telemetry/telemetry/un...
File tools/telemetry/telemetry/unittest_util/system_stub_unittest.py (right):

https://codereview.chromium.org/999963002/diff/1/tools/telemetry/telemetry/un...
tools/telemetry/telemetry/unittest_util/system_stub_unittest.py:227:
system_stub_test_module.SystemStubTest.TestOpen(file_path)
I assume you're testing that neither the stub nor the real call raises an
exception?

You might want to verify the contents after line 224 and/or verify that line 227
is actually returning a file object, or at least add comments that it's
sufficient that we don't get exceptions.

Otherwise, it's not obvious exactly what's being tested, and I'm a fan of
obviousness in unit tests :).

[aiolos (Not reviewing), 2015-03-12 19:01:16]

https://codereview.chromium.org/999963002/diff/1/tools/telemetry/telemetry/un...
File tools/telemetry/telemetry/unittest_util/system_stub_unittest.py (right):

https://codereview.chromium.org/999963002/diff/1/tools/telemetry/telemetry/un...
tools/telemetry/telemetry/unittest_util/system_stub_unittest.py:227:
system_stub_test_module.SystemStubTest.TestOpen(file_path)
On 2015/03/12 00:26:29, Dirk Pranke wrote:
> I assume you're testing that neither the stub nor the real call raises an
> exception?
> 
> You might want to verify the contents after line 224 and/or verify that line
227
> is actually returning a file object, or at least add comments that it's
> sufficient that we don't get exceptions.
> 
> Otherwise, it's not obvious exactly what's being tested, and I'm a fan of
> obviousness in unit tests :).

Done.

[sullivan, 2015-03-12 19:51:36]

sullivan@chromium.org changed reviewers:
+ sullivan@chromium.org

[sullivan, 2015-03-12 19:51:37]

lgtm

https://codereview.chromium.org/999963002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest_util/system_stub.py (right):

https://codereview.chromium.org/999963002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest_util/system_stub.py:52: if original_module is
None:
Would be good to have a comment here explaining how original_module could be
None

[aiolos (Not reviewing), 2015-03-12 20:26:12]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-03-12 20:26:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/999963002/#ps40001 (title:
"Add comment to Restore().")

[commit-bot: I haz the power, 2015-03-12 20:26:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/999963002/40001

[aiolos (Not reviewing), 2015-03-12 20:43:57]

The CQ bit was unchecked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-03-12 20:47:16]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-03-12 20:47:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/999963002/#ps40002 (title:
"Added Copyright info for Chromium presubmit.")

[commit-bot: I haz the power, 2015-03-12 20:47:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/999963002/40002

[commit-bot: I haz the power, 2015-03-12 22:05:35]

Committed patchset #4 (id:40002)

[commit-bot: I haz the power, 2015-03-12 22:06:43]

Patchset 4 (id:??) landed as
https://crrev.com/c12ba21289ad8a745e5526e547a78d92ab309ca8
Cr-Commit-Position: refs/heads/master@{#320385}