Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121823003:
Refactor test.py to be easier to fake. (Closed)
DescriptionRefactor test.py to be easier to fake.
Moved some of the data returned by TestPort into public instance
variables that can be replaced for testing.
BUG=595414
Committed: https://crrev.com/c3b32bfae7846d76e84f33f119d5762c9f556a35
Cr-Commit-Position: refs/heads/master@{#404212}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fixed comment #
Total comments: 1
Patch Set 3 : Updated with Dirk's suggestion #
Dependent Patchsets: Messages
Total messages: 17 (9 generated)
Description was changed from ========== [Update-test-expectations.py 1/3] Refactor test.py to be easier to fake. BUG=595414 ========== to ========== [Update-test-expectations.py 1/3] Refactor test.py to be easier to fake. BUG=595414 ==========
bokan@chromium.org changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
Description was changed from ========== [Update-test-expectations.py 1/3] Refactor test.py to be easier to fake. BUG=595414 ========== to ========== [Update-test-expectations.py 1/3] Refactor test.py to be easier to fake. BUG=595414 ==========
bokan@chromium.org changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (left): https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:546: ('trusty', 'x86_64')) Could this also be faked as-is without this CL? E.g. in a test method where you want to set `port._all_systems = (...)`, could you instead use `port._all_systems = lambda: (...)`? https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:401: """To avoid surprises when introducing new macros, these are intentionally fixed in time.""" Strings are usually only used for comments if they're at the top of a class or function or module -- because in these circumstances they get set as the __doc__ attribute of the class/function/module. In other places in the code, it's conventional to just use line comments that start with #.
https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (left): https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:546: ('trusty', 'x86_64')) On 2016/07/06 18:44:48, qyearsley wrote: > Could this also be faked as-is without this CL? > > E.g. in a test method where you want to set `port._all_systems = (...)`, could > you instead use `port._all_systems = lambda: (...)`? > > > Right, (Python isn't my native tongue :P) if you think that's better I can go ahead and change the tests and drop this CL. https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:401: """To avoid surprises when introducing new macros, these are intentionally fixed in time.""" On 2016/07/06 18:44:48, qyearsley wrote: > Strings are usually only used for comments if they're at the top of a class or > function or module -- because in these circumstances they get set as the __doc__ > attribute of the class/function/module. > > In other places in the code, it's conventional to just use line comments that > start with #. Thanks, done.
https://codereview.chromium.org/2121823003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): https://codereview.chromium.org/2121823003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:406: } I would make these public instance variables (i.e., set them in __init__(), instead, as self.all_systems, self.all_build_types, etc.) since you'll be updating them later.
https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (left): https://codereview.chromium.org/2121823003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:546: ('trusty', 'x86_64')) On 2016/07/06 at 21:29:19, bokan wrote: > On 2016/07/06 18:44:48, qyearsley wrote: > > Could this also be faked as-is without this CL? > > > > E.g. in a test method where you want to set `port._all_systems = (...)`, could > > you instead use `port._all_systems = lambda: (...)`? > > > > Right, (Python isn't my native tongue :P) if you think that's better I can go ahead and change the tests and drop this CL. I think this is fine -- I can't think any particular advantage of having these as functions rather than attributes, so this CL LGTM (once you've made the change Dirk suggested).
Description was changed from ========== [Update-test-expectations.py 1/3] Refactor test.py to be easier to fake. BUG=595414 ========== to ========== Refactor test.py to be easier to fake. Moved some of the data returned by TestPort into public instance variables that can be replaced for testing. BUG=595414 ==========
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2121823003/#ps40001 (title: "Updated with Dirk's suggestion")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor test.py to be easier to fake. Moved some of the data returned by TestPort into public instance variables that can be replaced for testing. BUG=595414 ========== to ========== Refactor test.py to be easier to fake. Moved some of the data returned by TestPort into public instance variables that can be replaced for testing. BUG=595414 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Refactor test.py to be easier to fake. Moved some of the data returned by TestPort into public instance variables that can be replaced for testing. BUG=595414 ========== to ========== Refactor test.py to be easier to fake. Moved some of the data returned by TestPort into public instance variables that can be replaced for testing. BUG=595414 Committed: https://crrev.com/c3b32bfae7846d76e84f33f119d5762c9f556a35 Cr-Commit-Position: refs/heads/master@{#404212} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c3b32bfae7846d76e84f33f119d5762c9f556a35 Cr-Commit-Position: refs/heads/master@{#404212} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
