Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398263003:
Added unittests for run_slave (Closed)
DescriptionAdded unittests for run_slave
BUG=651695
R=dsansome@google.com
CC=sergiyb@google.com
Committed: https://chromium.googlesource.com/chromium/tools/build/+/a25217b5d29342cd5df9a56c29fe52efe870e371
Patch Set 1 #Patch Set 2 : corrected the format of call_args for non-keyworded arguments #Patch Set 3 : removed debugging code #Patch Set 4 : reverted a change skipping other presubmit tests to shorten debugging time. #
Total comments: 22
Patch Set 5 : cosmetic fixes with more readable code #
Total comments: 4
Patch Set 6 : adjusted indentation #Patch Set 7 : cosmetic fix: added a blank link #Patch Set 8 : Merge branch 'master' into run_slave_unittests #Patch Set 9 : Run unittests for buildbot slaves within the PYTHONPATH overrides from environment.cfg.py #Patch Set 10 : Pass infra_path via PYTHONPATH env variable. #Messages
Total messages: 32 (20 generated)
dsansome@chromium.org changed reviewers: + dsansome@chromium.org
This looks really good! https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... File slave/tests/run_slave_test.py (right): https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:9: from mock import MagicMock, patch The style guide says not to import functions and classes from modules. Do "import mock" instead and use mock.MagicMock and mock.patch https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:19: print RUN_SLAVE_PATH Remove this line https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:26: sys.path.append(os.path.join(RUN_SLAVE_PATH, 'third_party', tw_ver)) Other tests use scripts/common/env.py to do this instead. sys.path.insert(0, os.path.join(os.path.dirname(__file__), os.pardir, ...)) # (/path/to/build/scripts) import common.env common.env.Install() https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:31: # This exeception is raised within a mocked os.execv() so that typo: s/exeception/exception/ https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:39: def MockExecv(executable_path, argv): Can you use mock.Mock instead? mock_execv = mock.Mock(side_effect=ExecvExecuted) ... self.assertIn(run_slave_py_path, mock_execv.call_args[0][0]) https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:44: # test if ryn_slave.py restarts itself after gclient sync typo: s/ryn/run/ https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:48: runpy.run_module("run_slave", run_name="__main__", alter_sys=True) Should be 2-space indentation here https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:54: self.assertTrue(cmd_args[0] == run_slave.GetGClientPath() and Avoid using assertTrue if possible. You'll get better error messages if you use assertEqual, and convention is to do assertEqual(expected, actual) self.assertEqual(run_slave.GetGClientPath(), cmd_args[0]) self.assertEqual('sync', cmd_args[1]) https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:66: with patch('subprocess.call') as subprocess_call, \ This is a bit long, consider using mock.patch as a decorator on the test function instead: @mock.patch('subprocess.check_call') @mock.patch('...') def test_run_slave_with_no_gclient_sync(self, mock_check_call, ...) https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:71: os.environ['TESTING_MASTER'] = 'Master1' Also 2-space indentation https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:81: self.assertFalse(gclient_path == cmd_args[0] and 'sync' == args[1]) Use assertEqual
ddoman@google.com changed reviewers: + ddoman@google.com
https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... File slave/tests/run_slave_test.py (right): https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:9: from mock import MagicMock, patch On 2016/10/10 05:25:18, dsansome wrote: > The style guide says not to import functions and classes from modules. Do > "import mock" instead and use mock.MagicMock and mock.patch Done. https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:19: print RUN_SLAVE_PATH On 2016/10/10 05:25:18, dsansome wrote: > Remove this line Done. https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:31: # This exeception is raised within a mocked os.execv() so that On 2016/10/10 05:25:18, dsansome wrote: > typo: s/exeception/exception/ Done. https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:39: def MockExecv(executable_path, argv): On 2016/10/10 05:25:18, dsansome wrote: > Can you use mock.Mock instead? > > mock_execv = mock.Mock(side_effect=ExecvExecuted) > ... > self.assertIn(run_slave_py_path, mock_execv.call_args[0][0]) Good point! https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:44: # test if ryn_slave.py restarts itself after gclient sync On 2016/10/10 05:25:18, dsansome wrote: > typo: s/ryn/run/ Done. https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:48: runpy.run_module("run_slave", run_name="__main__", alter_sys=True) On 2016/10/10 05:25:18, dsansome wrote: > Should be 2-space indentation here Done. https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:54: self.assertTrue(cmd_args[0] == run_slave.GetGClientPath() and On 2016/10/10 05:25:18, dsansome wrote: > Avoid using assertTrue if possible. You'll get better error messages if you use > assertEqual, and convention is to do assertEqual(expected, actual) > > self.assertEqual(run_slave.GetGClientPath(), cmd_args[0]) > self.assertEqual('sync', cmd_args[1]) Done. https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:66: with patch('subprocess.call') as subprocess_call, \ On 2016/10/10 05:25:18, dsansome wrote: > This is a bit long, consider using mock.patch as a decorator on the test > function instead: > > @mock.patch('subprocess.check_call') > @mock.patch('...') > def test_run_slave_with_no_gclient_sync(self, mock_check_call, ...) That looks much cleaner. Done https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:71: os.environ['TESTING_MASTER'] = 'Master1' On 2016/10/10 05:25:18, dsansome wrote: > Also 2-space indentation Done. https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:81: self.assertFalse(gclient_path == cmd_args[0] and 'sync' == args[1]) On 2016/10/10 05:25:18, dsansome wrote: > Use assertEqual Done.
lgtm https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... File slave/tests/run_slave_test.py (right): https://codereview.chromium.org/2398263003/diff/60001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:26: sys.path.append(os.path.join(RUN_SLAVE_PATH, 'third_party', tw_ver)) On 2016/10/10 05:25:18, dsansome wrote: > Other tests use scripts/common/env.py to do this instead. > > sys.path.insert(0, > os.path.join(os.path.dirname(__file__), os.pardir, ...)) > # (/path/to/build/scripts) > import common.env > common.env.Install() Did you try this? Did it work? https://codereview.chromium.org/2398263003/diff/80001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/2398263003/diff/80001/PRESUBMIT.py#newcode133 PRESUBMIT.py:133: 'slave', 'tests'), I'd put this on the line above https://codereview.chromium.org/2398263003/diff/80001/slave/tests/run_slave_t... File slave/tests/run_slave_test.py (right): https://codereview.chromium.org/2398263003/diff/80001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:38: 2 newlines between top-level definitions
https://codereview.chromium.org/2398263003/diff/80001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/2398263003/diff/80001/PRESUBMIT.py#newcode133 PRESUBMIT.py:133: 'slave', 'tests'), On 2016/10/12 03:54:04, dsansome wrote: > I'd put this on the line above Done. https://codereview.chromium.org/2398263003/diff/80001/slave/tests/run_slave_t... File slave/tests/run_slave_test.py (right): https://codereview.chromium.org/2398263003/diff/80001/slave/tests/run_slave_t... slave/tests/run_slave_test.py:38: On 2016/10/12 03:54:04, dsansome wrote: > 2 newlines between top-level definitions Done.
The CQ bit was checked by ddoman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dsansome@chromium.org Link to the patchset: https://codereview.chromium.org/2398263003/#ps140001 (title: "Merge branch 'master' into run_slave_unittests")
The CQ bit was unchecked by ddoman@google.com
The CQ bit was checked by ddoman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/32655c00b34d4a10)
The CQ bit was checked by ddoman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ddoman@google.com
The CQ bit was checked by ddoman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/329e24ab31f96710)
The CQ bit was checked by ddoman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dsansome@chromium.org Link to the patchset: https://codereview.chromium.org/2398263003/#ps160001 (title: "Run unittests for buildbot slaves within the PYTHONPATH overrides from environment.cfg.py")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/329ea53116324a10)
The CQ bit was checked by ddoman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dsansome@chromium.org Link to the patchset: https://codereview.chromium.org/2398263003/#ps180001 (title: "Pass infra_path via PYTHONPATH env variable.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1479715696879570,
"parent_rev": ["a043c267b8f3e153f4f06179a86cf517ac7abc70", null], "commit_rev":
["a25217b5d29342cd5df9a56c29fe52efe870e371", null]}
Message was sent while issue was closed.
Description was changed from ========== Added unittests for run_slave BUG=651695 R=dsansome@google.com CC=sergiyb@google.com ========== to ========== Added unittests for run_slave BUG=651695 R=dsansome@google.com CC=sergiyb@google.com Committed: https://chromium.googlesource.com/chromium/tools/build/+/a25217b5d29342cd5df9... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/tools/build/+/a25217b5d29342cd5df9... |
