Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1072443004:
[Telemetry] Add __enter__ & __exit__ methods for action_runner.Interaction. (Closed)
Description[Telemetry] Add __enter__ & __exit__ methods for action_runner.Interaction.
BUG=475090
Committed: https://crrev.com/9b9a362887abde2914254768c897fb0403eeacf1
Cr-Commit-Position: refs/heads/master@{#324315}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Add Returns in docstring #
Messages
Total messages: 26 (10 generated)
nednguyen@google.com changed reviewers: + dtu@chromium.org, eakuefner@chromium.org
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072443004/1
Patchset #1 (id:1) has been deleted
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072443004/20001
https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/actions/action_runner_unittest.py:18: import mock Don't have to fix it in this CL but I wonder if there's a better way of depending on mock at a project level since I imagine we'll probably use mock in future tests. https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/action_runner.py (right): https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/page/action_runner.py:106: have the same flags. I like Returns: just so it's immediately obvious that this function returns something from glancing at the docstring, but up to you. You could just say something like Returns: An action.Interaction instance as described, or even just Returns: an action.Interaction instance. https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/page/action_runner.py:135: have the same flags. Maybe here too.
whoops, lgtm :)
https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/actions/action_runner_unittest.py:18: import mock On 2015/04/08 20:24:01, eakuefner wrote: > Don't have to fix it in this CL but I wonder if there's a better way of > depending on mock at a project level since I imagine we'll probably use mock in > future tests. I would try to avoid import mock for all unittest. However, probably there is something we can do to replace "util.AddDirToPythonPath(util.GetTelemetryDir(), 'third_party', 'mock')" with "from telemetry.third_party import mock" https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/action_runner.py (right): https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/page/action_runner.py:106: have the same flags. On 2015/04/08 20:24:01, eakuefner wrote: > I like Returns: just so it's immediately obvious that this function returns > something from glancing at the docstring, but up to you. > > You could just say something like Returns: An action.Interaction instance as > described, or even just Returns: an action.Interaction instance. Done. https://codereview.chromium.org/1072443004/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/page/action_runner.py:135: have the same flags. On 2015/04/08 20:24:01, eakuefner wrote: > Maybe here too. Done.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/1072443004/#ps40001 (title: "Add Returns in docstring")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072443004/40001
The CQ bit was unchecked by commit-bot@chromium.org
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, _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 2015/04/08 20:31:04, I haz the power (commit-bot) wrote: > 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, > _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. +Dave: help~!
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
On 2015/04/08 at 20:31:04, commit-bot wrote: > 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, > _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. SOON
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072443004/40001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
This issue passed the CQ dry run.
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072443004/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9b9a362887abde2914254768c897fb0403eeacf1 Cr-Commit-Position: refs/heads/master@{#324315} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
