Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1998413002:
Simplify import-w3c-tests: support exactly one argument. (Closed)
|
Created:
4 years, 7 months ago by qyearsley Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews, tfarina, dcampb, raikiri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify import-w3c-tests: support exactly one argument.
Specifically, take the source repo path (top of repo) as the required argument, and don't support importing single subdirectories.
Reason:
1. Using the current directory as the top of repo may be a bit confusing.
2. In the future, I believe we want some kind of auto-updater that updates everything at once and we won't need to support importing individual directories. At present, I believe the only thing that uses import-w3c-tests is update-w3c-deps which, which doesn't currently have an option for updating just one subdirectory.
Committed: https://crrev.com/868f94e75ad4b86b2a8a6c15b694cde508af91ee
Cr-Commit-Position: refs/heads/master@{#398437}
Patch Set 1 #Patch Set 2 : Update usage string #
Total comments: 1
Patch Set 3 : Only support importing entire repo at once #Patch Set 4 : Update test_importer unit test #Patch Set 5 : Rebased #Patch Set 6 : Rebased #Patch Set 7 : Rebased #
Messages
Total messages: 35 (13 generated)
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py (right): https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:145: if len(args) > 2 or len(args) == 0: In what situation these days do we actually specify either 1 or 2 args? Can we just settle on one variant and not support both?
On 2016/05/23 at 20:28:24, dpranke wrote: > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py (right): > > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:145: if len(args) > 2 or len(args) == 0: > In what situation these days do we actually specify either 1 or 2 args? Can we just settle on one variant and not support both? Yes -- I was thinking maybe top_of_repo could be the one required positional arg and dir_to_import could be an option, does that sound OK?
On 2016/05/23 20:34:02, qyearsley wrote: > On 2016/05/23 at 20:28:24, dpranke wrote: > > > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py (right): > > > > > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:145: if > len(args) > 2 or len(args) == 0: > > In what situation these days do we actually specify either 1 or 2 args? Can we > just settle on one variant and not support both? > > Yes -- I was thinking maybe top_of_repo could be the one required positional arg > and dir_to_import could be an option, does that sound OK? That's the opposite of what I was suggesting. Do we need anything to be optional at all? Why not always require both arguments, or only support a single argument?
On 2016/05/23 at 20:39:39, dpranke wrote: > On 2016/05/23 20:34:02, qyearsley wrote: > > On 2016/05/23 at 20:28:24, dpranke wrote: > > > > > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py (right): > > > > > > > > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:145: if > > len(args) > 2 or len(args) == 0: > > > In what situation these days do we actually specify either 1 or 2 args? Can we > > just settle on one variant and not support both? > > > > Yes -- I was thinking maybe top_of_repo could be the one required positional arg > > and dir_to_import could be an option, does that sound OK? > > That's the opposite of what I was suggesting. Do we need anything to be optional at all? > Why not always require both arguments, or only support a single argument? Supporting only a single argument would be simpler; if we support two arguments, then I think there should be an optional argument to update-w3c-deps that allows one to update a single subdirectory; this might be useful although it's usefulness is limited if later we have an auto-updater bot or some other automatic import process. OK to change this CL to support only one argument in this CL?
On 2016/05/23 21:13:42, qyearsley wrote: > On 2016/05/23 at 20:39:39, dpranke wrote: > > On 2016/05/23 20:34:02, qyearsley wrote: > > > On 2016/05/23 at 20:28:24, dpranke wrote: > > > > > > > > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py > (right): > > > > > > > > > > > > https://codereview.chromium.org/1998413002/diff/20001/third_party/WebKit/Tool... > > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:145: if > > > len(args) > 2 or len(args) == 0: > > > > In what situation these days do we actually specify either 1 or 2 args? > Can we > > > just settle on one variant and not support both? > > > > > > Yes -- I was thinking maybe top_of_repo could be the one required positional > arg > > > and dir_to_import could be an option, does that sound OK? > > > > That's the opposite of what I was suggesting. Do we need anything to be > optional at all? > > Why not always require both arguments, or only support a single argument? > > Supporting only a single argument would be simpler; if we support two arguments, > then I think there should be an optional argument to update-w3c-deps that allows > one to update a single subdirectory; this might be useful although it's > usefulness is limited if later we have an auto-updater bot or some other > automatic import process. > > OK to change this CL to support only one argument in this CL? That's fine w/ me. You should make sure to tell anyone who does rolls about the change.
Description was changed from ========== Require at least one arg when invoking import-w3c-tests. Reason: Using the current directory as the top of repo may be a bit confusing. ========== to ========== Simplify import-w3c-tests: support exactly one argument. Specifically, take the source repo path (top of repo) as the required argument, and don't support importing single subdirectories. Reason: 1. Using the current directory as the top of repo may be a bit confusing. 2. In the future, I believe we want some kind of auto-updater that updates everything at once and we won't need to support importing individual directories. At present, I believe the only thing that uses import-w3c-tests is update-w3c-deps which, which doesn't currently have an option for updating just one subdirectory. ==========
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998413002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998413002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998413002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998413002/60001
qyearsley@chromium.org changed reviewers: + ojan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998413002/120001
On 2016/05/23 at 21:15:51, dpranke wrote: > > OK to change this CL to support only one argument in this CL? > > That's fine w/ me. You should make sure to tell anyone who does rolls about the change. I sent out an email to a bunch of folks that did rolls; Takayoshi Kochi and Koji Ishii both said OK, and nobody objected.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998413002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Note, this CL is ready for review; as noted above, I informed all people who have recently used the importer script and nobody objected. Does this look OK? (In retrospect, it might have been nicer to send that mail to blink-infra@chromium.org as well so that I could link to the mail from here.)
lgtm
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998413002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Simplify import-w3c-tests: support exactly one argument. Specifically, take the source repo path (top of repo) as the required argument, and don't support importing single subdirectories. Reason: 1. Using the current directory as the top of repo may be a bit confusing. 2. In the future, I believe we want some kind of auto-updater that updates everything at once and we won't need to support importing individual directories. At present, I believe the only thing that uses import-w3c-tests is update-w3c-deps which, which doesn't currently have an option for updating just one subdirectory. ========== to ========== Simplify import-w3c-tests: support exactly one argument. Specifically, take the source repo path (top of repo) as the required argument, and don't support importing single subdirectories. Reason: 1. Using the current directory as the top of repo may be a bit confusing. 2. In the future, I believe we want some kind of auto-updater that updates everything at once and we won't need to support importing individual directories. At present, I believe the only thing that uses import-w3c-tests is update-w3c-deps which, which doesn't currently have an option for updating just one subdirectory. Committed: https://crrev.com/868f94e75ad4b86b2a8a6c15b694cde508af91ee Cr-Commit-Position: refs/heads/master@{#398437} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/868f94e75ad4b86b2a8a6c15b694cde508af91ee Cr-Commit-Position: refs/heads/master@{#398437}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2048953002/ by kinuko@chromium.org. The reason for reverting is: This seems to have caused consecutive webkit python test failures. Probably there should be an easy fix but let me revert this for now. { "failures": [ "webkitpy/w3c/test_importer_unittest/TestImporterTest/test_does_not_import_reftest" ], "valid": true }.
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
