Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2554503004:
Add running unit tests to run-bindings-tests. (Closed)
DescriptionAdd running unit tests to run-bindings-tests.
Following up on the outcome of discussions on
https://codereview.chromium.org/2553503002, added a typ
runner at the end of run-bindings-tests. Now,
every unit test in Source/bindings/scripts will be
automatically ran by run-bindings-tests.
R=dpranke,bashi,yukishiino
BUG=654129
Committed: https://crrev.com/9ea86d59d0c3023efd5f0f01b1f7c43056f1a647
Cr-Commit-Position: refs/heads/master@{#437468}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Feedback addressed. #
Total comments: 3
Patch Set 3 : Landing in 4... 3... #
Messages
Total messages: 32 (19 generated)
The CQ bit was checked by dglazkov@chromium.org 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...
dglazkov@chromium.org changed reviewers: + yukishiino@chromium.org
PTAL.
LGTM
bashi@chromium.org changed reviewers: + bashi@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM. https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/run-bindings-tests (right): https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/run-bindings-tests:53: if return_code != 0: nit: Probably we'd like to run the unittests regardless of the result of run_bindings_tests and would like to see both failing results? Or typ.main first, then run_bindings_tests? https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py (right): https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py:11: def find_typ_dir(): nit: "find_typ_dir" reads to me that this function simply looks for something and doesn't make any side effect. Actually, the purpose of this function is to update the system path. Then, I'd recommend to rename this function to something like "add_typ_dir_to_sys_path". https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py:17: def find_bindings_scripts_dir(): ditto
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py (right): https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. There's a lot of overlap between this file and webkitpy/common/webkit_finder.py . Can we add logic to that file instead? https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py:11: def find_typ_dir(): On 2016/12/06 07:59:42, Yuki wrote: > nit: "find_typ_dir" reads to me that this function simply looks for something > and doesn't make any side effect. Actually, the purpose of this function is to > update the system path. Then, I'd recommend to rename this function to > something like "add_typ_dir_to_sys_path". Agreed.
On 2016/12/07 at 03:35:11, dpranke wrote: > https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... > File third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py (right): > > https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... > third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. > There's a lot of overlap between this file and webkitpy/common/webkit_finder.py . Can we add logic to that file instead? Yes! How hard do you want me to ape the conventions in that file? IOW, I sort of loathe replacing: from webkitpy import path_finder path_finder.find_bindings_scripts_dir() with: from webkitpy.common.webkit_finder import WebKitFinder from webkitpy.common.system.filesystem import FileSystem PATH_FINDER = WebKitFinder(FileSystem()) sys.path.append(PATH_FINDER.path_to_typ())
On 2016/12/07 15:36:19, dglazkov wrote: > On 2016/12/07 at 03:35:11, dpranke wrote: > > > https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... > > File third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py (right): > > > > > https://codereview.chromium.org/2554503004/diff/1/third_party/WebKit/Tools/Sc... > > third_party/WebKit/Tools/Scripts/webkitpy/path_finder.py:1: # Copyright 2016 > The Chromium Authors. All rights reserved. > > There's a lot of overlap between this file and > webkitpy/common/webkit_finder.py . Can we add logic to that file instead? > > Yes! How hard do you want me to ape the conventions in that file? IOW, I sort of > loathe replacing: > > from webkitpy import path_finder > path_finder.find_bindings_scripts_dir() > > with: > > from webkitpy.common.webkit_finder import WebKitFinder > from webkitpy.common.system.filesystem import FileSystem > PATH_FINDER = WebKitFinder(FileSystem()) > sys.path.append(PATH_FINDER.path_to_typ()) Adding a top level function that ran those four lines is fine, though I'd call it add_typ_to_sys_path(). -- Dirk
Feedback addressed.
The CQ bit was checked by dglazkov@chromium.org 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: This issue passed the CQ dry run.
lgtm. qyearsley/jeffcarp/tansell - fyi. https://codereview.chromium.org/2554503004/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/run-bindings-tests (left): https://codereview.chromium.org/2554503004/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/run-bindings-tests:49: Nit: put this line back in :). https://codereview.chromium.org/2554503004/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/run-bindings-tests (right): https://codereview.chromium.org/2554503004/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/run-bindings-tests:33: from webkitpy.bindings.bindings_tests import run_bindings_tests Nit: I think lines 29-31 actually should show up after line 33 (calling a function should show up after imports that run unconditionally). https://codereview.chromium.org/2554503004/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/test-webkitpy (right): https://codereview.chromium.org/2554503004/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/test-webkitpy:35: webkit_finder.add_typ_dir_to_sys_path() everybody wins!
Landing in 4... 3...
The CQ bit was checked by dglazkov@chromium.org 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: This issue passed the CQ dry run.
The CQ bit was checked by dglazkov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, haraken@chromium.org, bashi@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2554503004/#ps40001 (title: "Landing in 4... 3...")
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": 40001, "attempt_start_ts": 1481257997216560,
"parent_rev": "351b6aaa075667cf03efd45cfab790cdabb0b362", "commit_rev":
"3fb9d23a2fe0e6505ce01bfe4f7d0b0b89be26be"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add running unit tests to run-bindings-tests. Following up on the outcome of discussions on https://codereview.chromium.org/2553503002, added a typ runner at the end of run-bindings-tests. Now, every unit test in Source/bindings/scripts will be automatically ran by run-bindings-tests. R=dpranke,bashi,yukishiino BUG=654129 ========== to ========== Add running unit tests to run-bindings-tests. Following up on the outcome of discussions on https://codereview.chromium.org/2553503002, added a typ runner at the end of run-bindings-tests. Now, every unit test in Source/bindings/scripts will be automatically ran by run-bindings-tests. R=dpranke,bashi,yukishiino BUG=654129 Committed: https://crrev.com/9ea86d59d0c3023efd5f0f01b1f7c43056f1a647 Cr-Commit-Position: refs/heads/master@{#437468} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9ea86d59d0c3023efd5f0f01b1f7c43056f1a647 Cr-Commit-Position: refs/heads/master@{#437468} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
