Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1934533002:
Enable Skia patches to also run layout tests (Closed)
DescriptionEnable Skia patches to also run layout tests
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300334
Patch Set 1 : Intial upload #
Total comments: 2
Messages
Total messages: 20 (6 generated)
rmistry@google.com changed reviewers: + machenbach@chromium.org, tandrii@chromium.org
https://codereview.chromium.org/1934533002/diff/1/scripts/slave/recipes/chrom... File scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_blink_trybot.json (right): https://codereview.chromium.org/1934533002/diff/1/scripts/slave/recipes/chrom... scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_blink_trybot.json:235: "cwd": "[SLAVE_BUILD]/src", The diffs in this section are not what I expected. Did I do something wrong?
lgtm % I also don't know why it's there. But, i think it's totally find to land and see.
On 2016/04/29 11:07:38, tandrii(chromium) wrote: > lgtm % I also don't know why it's there. But, i think it's totally find to land > and see. SG. Landing..
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934533002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934533002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934533002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934533002/1
Message was sent while issue was closed.
Description was changed from ========== Enable Skia patches to also run layout tests BUG= ========== to ========== Enable Skia patches to also run layout tests BUG= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300334 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300334
Message was sent while issue was closed.
https://codereview.chromium.org/1934533002/diff/1/scripts/slave/recipes/chrom... File scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_blink_trybot.json (right): https://codereview.chromium.org/1934533002/diff/1/scripts/slave/recipes/chrom... scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_blink_trybot.json:235: "cwd": "[SLAVE_BUILD]/src", On 2016/04/29 11:04:03, rmistry wrote: > The diffs in this section are not what I expected. Did I do something wrong? Hmm - that's not right indeed. Wonder if it's just simulated wrong. Maybe just try out in prod and check. I dug a bit but couldn't find right now if I did anything in v8 to get it to the right cwd...
Message was sent while issue was closed.
FYI: The diff feature was added during this migration: https://chromium.googlesource.com/chromium/tools/build/+/48ead6a51947ac5fc070... The old blink test case doesn't have it, the new chromium one has it with the correct v8 cwd. No idea why...
Message was sent while issue was closed.
On 2016/04/29 11:56:01, Michael Achenbach wrote: > FYI: The diff feature was added during this migration: > https://chromium.googlesource.com/chromium/tools/build/+/48ead6a51947ac5fc070... > > The old blink test case doesn't have it, the new chromium one has it with the > correct v8 cwd. No idea why... Does not look like its working: https://codereview.chromium.org/1934513003/ I do not see layout tests run there.
Message was sent while issue was closed.
On 2016/04/29 11:57:58, rmistry wrote: > On 2016/04/29 11:56:01, Michael Achenbach wrote: > > FYI: The diff feature was added during this migration: > > > https://chromium.googlesource.com/chromium/tools/build/+/48ead6a51947ac5fc070... > > > > The old blink test case doesn't have it, the new chromium one has it with the > > correct v8 cwd. No idea why... > > Does not look like its working: > https://codereview.chromium.org/1934513003/ > I do not see layout tests run there. So the expectations match prod. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... is in the src directory which is wrong.
Message was sent while issue was closed.
Unfortunately this method is still used for the diff tool: https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/reci... And it calculates the wrong issue root. Gclients new method that tandrii added should be used instead here: https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/reci...
Message was sent while issue was closed.
On 2016/04/29 12:06:49, Michael Achenbach wrote: > Unfortunately this method is still used for the diff tool: > https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/reci... > > And it calculates the wrong issue root. Gclients new method that tandrii added > should be used instead here: > https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/reci... Thanks for looking into this! Andrii, do you mind updating that in depot_tools?
Message was sent while issue was closed.
On 2016/04/29 12:10:25, rmistry wrote: > On 2016/04/29 12:06:49, Michael Achenbach wrote: > > Unfortunately this method is still used for the diff tool: > > > https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/reci... > > > > And it calculates the wrong issue root. Gclients new method that tandrii added > > should be used instead here: > > > https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/reci... > > Thanks for looking into this! > > Andrii, do you mind updating that in depot_tools? yep, on it. But I can't use gclient's method from inside tryserver due to cyclic dependency :(
Message was sent while issue was closed.
Patchset #2 (id:20001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
