|
|
DescriptionUse LocalWPT.test_patch when filtering exportable commits
(3/5) This CL is based off of https://codereview.chromium.org/2576353003 and needs to be rebased once that is landed.
BUG=657117
R=qyearsley@chromium.org
Committed: https://crrev.com/55d6439cbf7534623a716df05eb24c7b88cff3a9
Cr-Commit-Position: refs/heads/master@{#440572}
Patch Set 1 #Patch Set 2 : Add missing constant #
Total comments: 7
Patch Set 3 : Address CL feedback #Patch Set 4 : Add CL suggestion, need to fix unit tests #Patch Set 5 : Fix test_exporter_unittest #
Messages
Total messages: 20 (9 generated)
Description was changed from ========== Use LocalWPT.test_patch when filtering exportable commits BUG=657117 R=qyearsley@chromium.org ========== to ========== Use LocalWPT.test_patch when filtering exportable commits (3/5) This CL is based off of https://codereview.chromium.org/2576353003 and needs to be rebased once that is landed. BUG=657117 R=qyearsley@chromium.org ==========
https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:112: """Returns the expected output of a patch agains origin/master. Nit: agains -> against https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:118: A string containing the diff the patch produced. Sometimes I'm confused about the words "diff" and "patch". Sometimes a patch is a diff and a diff is a patch (i.e. the output of `git diff` or `diff`), but there are different types of diffs and patch can sometimes mean other things. Even if this method isn't very testable because it because involves interacting with external systems (the filesystem and git subprocesses), it might be nice to see a simple example input and output. Also: I'm not sure if it's a good idea, but in scm_unittest.py, it actually creates a real git repo in a temp dir to do testing (see: https://codereview.chromium.org/2594513003). That kind of test may also be possible for some methods of LocalWPT? https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:123: # TODO(jeffcarp): dedupe Dedupe what? https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py (right): https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:82: return '' This is definitely fine, although doing something using a dict like the other methods would also work, with a bit of tweaking, e.g.: canned_git_outputs = { 'show': 'newer fake text' if 'cafedad5' in args else 'older fake text', 'rev-list': 'facebeef\ncafedad5', 'footers': 'fake-cr-position', 'remote': 'github', 'format-patch': 'fake patch', 'diff': 'fake patch diff', 'diff-tree': 'fake\n\files\nchanged', } return canned_git_outputs.get(git_command, '')
https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:118: A string containing the diff the patch produced. On 2016/12/22 at 22:04:54, qyearsley wrote: > Sometimes I'm confused about the words "diff" and "patch". > > Sometimes a patch is a diff and a diff is a patch (i.e. the output of `git diff` or `diff`), but there are different types of diffs and patch can sometimes mean other things. > > Even if this method isn't very testable because it because involves interacting with external systems (the filesystem and git subprocesses), it might be nice to see a simple example input and output. > > Also: I'm not sure if it's a good idea, but in scm_unittest.py, it actually creates a real git repo in a temp dir to do testing (see: https://codereview.chromium.org/2594513003). That kind of test may also be possible for some methods of LocalWPT? Hm what do you suggest re: diff/patch? That sounds like a good idea but I'd like to do that after I transfer things to webkitpy.common.checkout.git.Git. I can make a bug for it. https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:123: # TODO(jeffcarp): dedupe On 2016/12/22 at 22:04:54, qyearsley wrote: > Dedupe what? Oops it was already deduped.
https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:118: A string containing the diff the patch produced. On 2016/12/22 at 23:14:29, jeffcarp wrote: > On 2016/12/22 at 22:04:54, qyearsley wrote: > > Sometimes I'm confused about the words "diff" and "patch". > > > > Sometimes a patch is a diff and a diff is a patch (i.e. the output of `git diff` or `diff`), but there are different types of diffs and patch can sometimes mean other things. > > > > Even if this method isn't very testable because it because involves interacting with external systems (the filesystem and git subprocesses), it might be nice to see a simple example input and output. > > > > Also: I'm not sure if it's a good idea, but in scm_unittest.py, it actually creates a real git repo in a temp dir to do testing (see: https://codereview.chromium.org/2594513003). That kind of test may also be possible for some methods of LocalWPT? > > Hm what do you suggest re: diff/patch? I was wondering if you could put an example (very short) input and output that this might produce, depending on the state of origin/master -- if the files in WPT at origin/master is the same as Chromium was before the Chromium CL, then the input and output would be very similar, right? But then if WPT has been updated since the time when the Chromium CL was made, then there may or may not be a conflict, and if there's no conflict, the output diff might be different from the input diff?
On 2016/12/22 at 23:19:53, qyearsley wrote: > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): > > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:118: A string containing the diff the patch produced. > On 2016/12/22 at 23:14:29, jeffcarp wrote: > > On 2016/12/22 at 22:04:54, qyearsley wrote: > > > Sometimes I'm confused about the words "diff" and "patch". > > > > > > Sometimes a patch is a diff and a diff is a patch (i.e. the output of `git diff` or `diff`), but there are different types of diffs and patch can sometimes mean other things. > > > > > > Even if this method isn't very testable because it because involves interacting with external systems (the filesystem and git subprocesses), it might be nice to see a simple example input and output. > > > > > > Also: I'm not sure if it's a good idea, but in scm_unittest.py, it actually creates a real git repo in a temp dir to do testing (see: https://codereview.chromium.org/2594513003). That kind of test may also be possible for some methods of LocalWPT? > > > > Hm what do you suggest re: diff/patch? > > I was wondering if you could put an example (very short) input and output that this might produce, depending on the state of origin/master -- if the files in WPT at origin/master is the same as Chromium was before the Chromium CL, then the input and output would be very similar, right? > > But then if WPT has been updated since the time when the Chromium CL was made, then there may or may not be a conflict, and if there's no conflict, the output diff might be different from the input diff? Addressed feedback - need to fix some unit tests before this will be green.
On 2016/12/22 at 23:25:18, jeffcarp wrote: > On 2016/12/22 at 23:19:53, qyearsley wrote: > > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): > > > > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:118: A string containing the diff the patch produced. > > On 2016/12/22 at 23:14:29, jeffcarp wrote: > > > On 2016/12/22 at 22:04:54, qyearsley wrote: > > > > Sometimes I'm confused about the words "diff" and "patch". > > > > > > > > Sometimes a patch is a diff and a diff is a patch (i.e. the output of `git diff` or `diff`), but there are different types of diffs and patch can sometimes mean other things. > > > > > > > > Even if this method isn't very testable because it because involves interacting with external systems (the filesystem and git subprocesses), it might be nice to see a simple example input and output. > > > > > > > > Also: I'm not sure if it's a good idea, but in scm_unittest.py, it actually creates a real git repo in a temp dir to do testing (see: https://codereview.chromium.org/2594513003). That kind of test may also be possible for some methods of LocalWPT? > > > > > > Hm what do you suggest re: diff/patch? > > > > I was wondering if you could put an example (very short) input and output that this might produce, depending on the state of origin/master -- if the files in WPT at origin/master is the same as Chromium was before the Chromium CL, then the input and output would be very similar, right? > > > > But then if WPT has been updated since the time when the Chromium CL was made, then there may or may not be a conflict, and if there's no conflict, the output diff might be different from the input diff? > > Addressed feedback - need to fix some unit tests before this will be green. Unit tests should be green now.
The CQ bit was checked by jeffcarp@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...
On 2016/12/22 at 23:40:56, jeffcarp wrote: > On 2016/12/22 at 23:25:18, jeffcarp wrote: > > On 2016/12/22 at 23:19:53, qyearsley wrote: > > > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): > > > > > > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:118: A string containing the diff the patch produced. > > > On 2016/12/22 at 23:14:29, jeffcarp wrote: > > > > On 2016/12/22 at 22:04:54, qyearsley wrote: > > > > > Sometimes I'm confused about the words "diff" and "patch". > > > > > > > > > > Sometimes a patch is a diff and a diff is a patch (i.e. the output of `git diff` or `diff`), but there are different types of diffs and patch can sometimes mean other things. > > > > > > > > > > Even if this method isn't very testable because it because involves interacting with external systems (the filesystem and git subprocesses), it might be nice to see a simple example input and output. > > > > > > > > > > Also: I'm not sure if it's a good idea, but in scm_unittest.py, it actually creates a real git repo in a temp dir to do testing (see: https://codereview.chromium.org/2594513003). That kind of test may also be possible for some methods of LocalWPT? > > > > > > > > Hm what do you suggest re: diff/patch? > > > > > > I was wondering if you could put an example (very short) input and output that this might produce, depending on the state of origin/master -- if the files in WPT at origin/master is the same as Chromium was before the Chromium CL, then the input and output would be very similar, right? > > > > > > But then if WPT has been updated since the time when the Chromium CL was made, then there may or may not be a conflict, and if there's no conflict, the output diff might be different from the input diff? > > > > Addressed feedback - need to fix some unit tests before this will be green. > > Unit tests should be green now. LGTM to hit the CQ; I'm still not very clear about LocalWPT.test_patch(), although (I think) I see that it's main purpose is to check whether a patch will be able to apply cleanly to WPT origin/master (right?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/22 at 23:44:58, qyearsley wrote: > On 2016/12/22 at 23:40:56, jeffcarp wrote: > > On 2016/12/22 at 23:25:18, jeffcarp wrote: > > > On 2016/12/22 at 23:19:53, qyearsley wrote: > > > > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > > > > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right): > > > > > > > > https://codereview.chromium.org/2583723002/diff/20001/third_party/WebKit/Tool... > > > > third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:118: A string containing the diff the patch produced. > > > > On 2016/12/22 at 23:14:29, jeffcarp wrote: > > > > > On 2016/12/22 at 22:04:54, qyearsley wrote: > > > > > > Sometimes I'm confused about the words "diff" and "patch". > > > > > > > > > > > > Sometimes a patch is a diff and a diff is a patch (i.e. the output of `git diff` or `diff`), but there are different types of diffs and patch can sometimes mean other things. > > > > > > > > > > > > Even if this method isn't very testable because it because involves interacting with external systems (the filesystem and git subprocesses), it might be nice to see a simple example input and output. > > > > > > > > > > > > Also: I'm not sure if it's a good idea, but in scm_unittest.py, it actually creates a real git repo in a temp dir to do testing (see: https://codereview.chromium.org/2594513003). That kind of test may also be possible for some methods of LocalWPT? > > > > > > > > > > Hm what do you suggest re: diff/patch? > > > > > > > > I was wondering if you could put an example (very short) input and output that this might produce, depending on the state of origin/master -- if the files in WPT at origin/master is the same as Chromium was before the Chromium CL, then the input and output would be very similar, right? > > > > > > > > But then if WPT has been updated since the time when the Chromium CL was made, then there may or may not be a conflict, and if there's no conflict, the output diff might be different from the input diff? > > > > > > Addressed feedback - need to fix some unit tests before this will be green. > > > > Unit tests should be green now. > > > LGTM to hit the CQ; I'm still not very clear about LocalWPT.test_patch(), although (I think) I see that it's main purpose is to check whether a patch will be able to apply cleanly to WPT origin/master (right?) Yep that's right!
The CQ bit was checked by jeffcarp@chromium.org
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": 80001, "attempt_start_ts": 1482455541922000, "parent_rev": "2d56efae16f43753f49c2314780cbd78ffe78b0a", "commit_rev": "1b0ca008b52ffeb617edc7401f5da4de25780aa9"}
Message was sent while issue was closed.
Description was changed from ========== Use LocalWPT.test_patch when filtering exportable commits (3/5) This CL is based off of https://codereview.chromium.org/2576353003 and needs to be rebased once that is landed. BUG=657117 R=qyearsley@chromium.org ========== to ========== Use LocalWPT.test_patch when filtering exportable commits (3/5) This CL is based off of https://codereview.chromium.org/2576353003 and needs to be rebased once that is landed. BUG=657117 R=qyearsley@chromium.org Review-Url: https://codereview.chromium.org/2583723002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Use LocalWPT.test_patch when filtering exportable commits (3/5) This CL is based off of https://codereview.chromium.org/2576353003 and needs to be rebased once that is landed. BUG=657117 R=qyearsley@chromium.org Review-Url: https://codereview.chromium.org/2583723002 ========== to ========== Use LocalWPT.test_patch when filtering exportable commits (3/5) This CL is based off of https://codereview.chromium.org/2576353003 and needs to be rebased once that is landed. BUG=657117 R=qyearsley@chromium.org Committed: https://crrev.com/55d6439cbf7534623a716df05eb24c7b88cff3a9 Cr-Commit-Position: refs/heads/master@{#440572} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/55d6439cbf7534623a716df05eb24c7b88cff3a9 Cr-Commit-Position: refs/heads/master@{#440572} |