Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19540019:
Tests for changes to Blink in Issues 19697008. This adds new javascript to the (Closed)
|
Created:
7 years, 5 months ago by tmroeder Modified:
7 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionTests for changes to Blink in Issues 19697008. This adds new javascript to the
friend and test extensions to exercise the new activity log events for keyboard
and mouse.
BUG=260336
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214447
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixes as per comments from mpcomplete #Patch Set 3 : Fix to finish renaming domExpectedActivity2 #
Messages
Total messages: 26 (0 generated)
These are the tests for the Blink change in Issue 19697008 that add many new events for the Activity Log. The tests are currently failing, since they depend on that CL. I have run these tests successfully in Chromium source tree with the changes from the other CL.
On 2013/07/18 23:04:58, tmroeder wrote: > These are the tests for the Blink change in Issue 19697008 that add many new > events for the Activity Log. The tests are currently failing, since they depend > on that CL. I have run these tests successfully in Chromium source tree with the > changes from the other CL. Hi Tom, the trybots are showing failures that look to be caused by this CL, could you check that out?
On 2013/07/19 01:13:58, felt wrote: > On 2013/07/18 23:04:58, tmroeder wrote: > > These are the tests for the Blink change in Issue 19697008 that add many new > > events for the Activity Log. The tests are currently failing, since they > depend > > on that CL. I have run these tests successfully in Chromium source tree with > the > > changes from the other CL. > > Hi Tom, the trybots are showing failures that look to be caused by this CL, > could you check that out? This CL hasn't landed, AFAIK. And third_party/WebKit in the Chromium tree hasn't yet picked up my Blink change. As I write this, the tip of Blink in Chromium is at 154493, while my change is at 154552
On 2013/07/19 03:22:29, tmroeder wrote: > On 2013/07/19 01:13:58, felt wrote: > > On 2013/07/18 23:04:58, tmroeder wrote: > > > These are the tests for the Blink change in Issue 19697008 that add many new > > > events for the Activity Log. The tests are currently failing, since they > > depend > > > on that CL. I have run these tests successfully in Chromium source tree with > > the > > > changes from the other CL. > > > > Hi Tom, the trybots are showing failures that look to be caused by this CL, > > could you check that out? > > This CL hasn't landed, AFAIK. And third_party/WebKit in the Chromium tree hasn't > yet picked up my Blink change. As I write this, the tip of Blink in Chromium is > at 154493, while my change is at 154552 I was referring to the other change (I was concerned it would break at least the geolocation test) -- but apparently that's not the case. :) Thanks! Can you add the geolocation event I asked about? Perhaps it might make sense to comment out that test case in this CL so that after the geolocation logging one lands in Blink, it doesn't cause a failure. (Then in another CL after that second blink patch lands, re-enable that test case.)
On 2013/07/19 06:15:38, felt wrote: > On 2013/07/19 03:22:29, tmroeder wrote: > > On 2013/07/19 01:13:58, felt wrote: > > > On 2013/07/18 23:04:58, tmroeder wrote: > > > > These are the tests for the Blink change in Issue 19697008 that add many > new > > > > events for the Activity Log. The tests are currently failing, since they > > > depend > > > > on that CL. I have run these tests successfully in Chromium source tree > with > > > the > > > > changes from the other CL. > > > > > > Hi Tom, the trybots are showing failures that look to be caused by this CL, > > > could you check that out? > > > > This CL hasn't landed, AFAIK. And third_party/WebKit in the Chromium tree > hasn't > > yet picked up my Blink change. As I write this, the tip of Blink in Chromium > is > > at 154493, while my change is at 154552 > > I was referring to the other change (I was concerned it would break at least the > geolocation test) -- but apparently that's not the case. :) Thanks! > > Can you add the geolocation event I asked about? Perhaps it might make sense to > comment out that test case in this CL so that after the geolocation logging one > lands in Blink, it doesn't cause a failure. (Then in another CL after that > second blink patch lands, re-enable that test case.) P.S. There are still trybot failures due to this CL, do you know what's causing them? (I mean failures on this patch's CL runs, not patches on the main ToT.)
On 2013/07/19 06:17:45, felt wrote: > On 2013/07/19 06:15:38, felt wrote: > > On 2013/07/19 03:22:29, tmroeder wrote: > > > On 2013/07/19 01:13:58, felt wrote: > > > > On 2013/07/18 23:04:58, tmroeder wrote: > > > > > These are the tests for the Blink change in Issue 19697008 that add many > > new > > > > > events for the Activity Log. The tests are currently failing, since they > > > > depend > > > > > on that CL. I have run these tests successfully in Chromium source tree > > with > > > > the > > > > > changes from the other CL. > > > > > > > > Hi Tom, the trybots are showing failures that look to be caused by this > CL, > > > > could you check that out? > > > > > > This CL hasn't landed, AFAIK. And third_party/WebKit in the Chromium tree > > hasn't > > > yet picked up my Blink change. As I write this, the tip of Blink in Chromium > > is > > > at 154493, while my change is at 154552 > > > > I was referring to the other change (I was concerned it would break at least > the > > geolocation test) -- but apparently that's not the case. :) Thanks! > > > > Can you add the geolocation event I asked about? Perhaps it might make sense > to > > comment out that test case in this CL so that after the geolocation logging > one > > lands in Blink, it doesn't cause a failure. (Then in another CL after that > > second blink patch lands, re-enable that test case.) > > P.S. There are still trybot failures due to this CL, do you know what's causing > them? (I mean failures on this patch's CL runs, not patches on the main ToT.) *this patch's trybot runs. Sorry, I am extremely jetlagged...
On 2013/07/19 06:18:13, felt wrote: > On 2013/07/19 06:17:45, felt wrote: > > On 2013/07/19 06:15:38, felt wrote: > > > On 2013/07/19 03:22:29, tmroeder wrote: > > > > On 2013/07/19 01:13:58, felt wrote: > > > > > On 2013/07/18 23:04:58, tmroeder wrote: > > > > > > These are the tests for the Blink change in Issue 19697008 that add > many > > > new > > > > > > events for the Activity Log. The tests are currently failing, since > they > > > > > depend > > > > > > on that CL. I have run these tests successfully in Chromium source > tree > > > with > > > > > the > > > > > > changes from the other CL. > > > > > > > > > > Hi Tom, the trybots are showing failures that look to be caused by this > > CL, > > > > > could you check that out? > > > > > > > > This CL hasn't landed, AFAIK. And third_party/WebKit in the Chromium tree > > > hasn't > > > > yet picked up my Blink change. As I write this, the tip of Blink in > Chromium > > > is > > > > at 154493, while my change is at 154552 > > > > > > I was referring to the other change (I was concerned it would break at least > > the > > > geolocation test) -- but apparently that's not the case. :) Thanks! > > > > > > Can you add the geolocation event I asked about? Perhaps it might make sense > > to > > > comment out that test case in this CL so that after the geolocation logging > > one > > > lands in Blink, it doesn't cause a failure. (Then in another CL after that > > > second blink patch lands, re-enable that test case.) > > > > P.S. There are still trybot failures due to this CL, do you know what's > causing > > them? (I mean failures on this patch's CL runs, not patches on the main ToT.) > > *this patch's trybot runs. Sorry, I am extremely jetlagged... Yes. This patch can't work until my changes to Blink roll into Chromium.
On 2013/07/19 06:15:38, felt wrote: > On 2013/07/19 03:22:29, tmroeder wrote: > > On 2013/07/19 01:13:58, felt wrote: > > > On 2013/07/18 23:04:58, tmroeder wrote: > > > > These are the tests for the Blink change in Issue 19697008 that add many > new > > > > events for the Activity Log. The tests are currently failing, since they > > > depend > > > > on that CL. I have run these tests successfully in Chromium source tree > with > > > the > > > > changes from the other CL. > > > > > > Hi Tom, the trybots are showing failures that look to be caused by this CL, > > > could you check that out? > > > > This CL hasn't landed, AFAIK. And third_party/WebKit in the Chromium tree > hasn't > > yet picked up my Blink change. As I write this, the tip of Blink in Chromium > is > > at 154493, while my change is at 154552 > > I was referring to the other change (I was concerned it would break at least the > geolocation test) -- but apparently that's not the case. :) Thanks! > > Can you add the geolocation event I asked about? Perhaps it might make sense to > comment out that test case in this CL so that after the geolocation logging one > lands in Blink, it doesn't cause a failure. (Then in another CL after that > second blink patch lands, re-enable that test case.) Will do.
I think I'm too jetlagged to review any more code without screwing up, so recusing myself-- can you please ask mpcomplete to review, once you've checked the new tests work with the updated version of blink?
On 2013/07/19 21:28:24, felt wrote: > I think I'm too jetlagged to review any more code without screwing up, so > recusing myself-- can you please ask mpcomplete to review, once you've checked > the new tests work with the updated version of blink? +mpcomplete The tests are all passing now other than mac_asan, and the failure there looks entirely unrelated: it's failing due to some temp file problem while trying to do gclient revert.
lgtm https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... File chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js (right): https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:327: // tests for hooking keyboard and mouse handlers Use full sentences for comments. https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... File chrome/test/data/extensions/api_test/activity_log_private/test/test.js (right): https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... chrome/test/data/extensions/api_test/activity_log_private/test/test.js:291: var domExpectedActivity2 = domExpectedActivity.slice(0); "domExpectedActivityIncognito" would make it clearer that it's for the incognito test.
On 2013/07/22 18:07:04, Matt Perry wrote: > lgtm > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > File chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js > (right): > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:327: > // tests for hooking keyboard and mouse handlers > Use full sentences for comments. > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > File chrome/test/data/extensions/api_test/activity_log_private/test/test.js > (right): > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > chrome/test/data/extensions/api_test/activity_log_private/test/test.js:291: var > domExpectedActivity2 = domExpectedActivity.slice(0); > "domExpectedActivityIncognito" would make it clearer that it's for the incognito > test. Fixed, and set up patch set 2 above. However, git cl try now crashes in my local repo, and many of the bots above seem to be failing, though not on anything to do with my change. How should I proceed?
On 2013/07/24 18:17:24, tmroeder wrote: > On 2013/07/22 18:07:04, Matt Perry wrote: > > lgtm > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > File chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js > > (right): > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:327: > > // tests for hooking keyboard and mouse handlers > > Use full sentences for comments. > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > File chrome/test/data/extensions/api_test/activity_log_private/test/test.js > > (right): > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > chrome/test/data/extensions/api_test/activity_log_private/test/test.js:291: > var > > domExpectedActivity2 = domExpectedActivity.slice(0); > > "domExpectedActivityIncognito" would make it clearer that it's for the > incognito > > test. > > Fixed, and set up patch set 2 above. > > However, git cl try now crashes in my local repo, and many of the bots above > seem to be failing, though not on anything to do with my change. How should I > proceed? Let's try the CQ one more time. If it fails, go ahead and git cl dcommit manually.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmroeder@chromium.org/19540019/27001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
On 2013/07/24 18:36:31, Matt Perry wrote: > On 2013/07/24 18:17:24, tmroeder wrote: > > On 2013/07/22 18:07:04, Matt Perry wrote: > > > lgtm > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > File > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js > > > (right): > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:327: > > > // tests for hooking keyboard and mouse handlers > > > Use full sentences for comments. > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > File chrome/test/data/extensions/api_test/activity_log_private/test/test.js > > > (right): > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > chrome/test/data/extensions/api_test/activity_log_private/test/test.js:291: > > var > > > domExpectedActivity2 = domExpectedActivity.slice(0); > > > "domExpectedActivityIncognito" would make it clearer that it's for the > > incognito > > > test. > > > > Fixed, and set up patch set 2 above. > > > > However, git cl try now crashes in my local repo, and many of the bots above > > seem to be failing, though not on anything to do with my change. How should I > > proceed? > > Let's try the CQ one more time. If it fails, go ahead and git cl dcommit > manually. Wait, please don't dcommit. The failures look related to your patch: http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/1519...
On 2013/07/24 21:37:27, felt wrote: > On 2013/07/24 18:36:31, Matt Perry wrote: > > On 2013/07/24 18:17:24, tmroeder wrote: > > > On 2013/07/22 18:07:04, Matt Perry wrote: > > > > lgtm > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > File > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js > > > > (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:327: > > > > // tests for hooking keyboard and mouse handlers > > > > Use full sentences for comments. > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > File > chrome/test/data/extensions/api_test/activity_log_private/test/test.js > > > > (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > chrome/test/data/extensions/api_test/activity_log_private/test/test.js:291: > > > var > > > > domExpectedActivity2 = domExpectedActivity.slice(0); > > > > "domExpectedActivityIncognito" would make it clearer that it's for the > > > incognito > > > > test. > > > > > > Fixed, and set up patch set 2 above. > > > > > > However, git cl try now crashes in my local repo, and many of the bots above > > > seem to be failing, though not on anything to do with my change. How should > I > > > proceed? > > > > Let's try the CQ one more time. If it fails, go ahead and git cl dcommit > > manually. > > Wait, please don't dcommit. The failures look related to your patch: > http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/1519... Actually, I just noticed that there is a bug here... I forgot to rename one of the instances of domExpectedActivity2. About to submit a new patch.
On 2013/07/24 21:40:39, tmroeder wrote: > On 2013/07/24 21:37:27, felt wrote: > > On 2013/07/24 18:36:31, Matt Perry wrote: > > > On 2013/07/24 18:17:24, tmroeder wrote: > > > > On 2013/07/22 18:07:04, Matt Perry wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > File > > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > > > > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:327: > > > > > // tests for hooking keyboard and mouse handlers > > > > > Use full sentences for comments. > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > File > > chrome/test/data/extensions/api_test/activity_log_private/test/test.js > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > > > chrome/test/data/extensions/api_test/activity_log_private/test/test.js:291: > > > > var > > > > > domExpectedActivity2 = domExpectedActivity.slice(0); > > > > > "domExpectedActivityIncognito" would make it clearer that it's for the > > > > incognito > > > > > test. > > > > > > > > Fixed, and set up patch set 2 above. > > > > > > > > However, git cl try now crashes in my local repo, and many of the bots > above > > > > seem to be failing, though not on anything to do with my change. How > should > > I > > > > proceed? > > > > > > Let's try the CQ one more time. If it fails, go ahead and git cl dcommit > > > manually. > > > > Wait, please don't dcommit. The failures look related to your patch: > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/1519... > > Actually, I just noticed that there is a bug here... I forgot to rename one of > the instances of domExpectedActivity2. About to submit a new patch. OK, now I don't see any test failures that look related to the patch, though there do seem to be a bunch of unrelated browser_tests failing. Is that normal? In the cases I can see where the ActivityLogApiTest.TriggerEvent ran, it succeeded.
On 2013/07/24 22:56:03, tmroeder wrote: > On 2013/07/24 21:40:39, tmroeder wrote: > > On 2013/07/24 21:37:27, felt wrote: > > > On 2013/07/24 18:36:31, Matt Perry wrote: > > > > On 2013/07/24 18:17:24, tmroeder wrote: > > > > > On 2013/07/22 18:07:04, Matt Perry wrote: > > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > > File > > > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > > > > > > > > chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:327: > > > > > > // tests for hooking keyboard and mouse handlers > > > > > > Use full sentences for comments. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > > File > > > chrome/test/data/extensions/api_test/activity_log_private/test/test.js > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/19540019/diff/1/chrome/test/data/exten... > > > > > > > > > chrome/test/data/extensions/api_test/activity_log_private/test/test.js:291: > > > > > var > > > > > > domExpectedActivity2 = domExpectedActivity.slice(0); > > > > > > "domExpectedActivityIncognito" would make it clearer that it's for the > > > > > incognito > > > > > > test. > > > > > > > > > > Fixed, and set up patch set 2 above. > > > > > > > > > > However, git cl try now crashes in my local repo, and many of the bots > > above > > > > > seem to be failing, though not on anything to do with my change. How > > should > > > I > > > > > proceed? > > > > > > > > Let's try the CQ one more time. If it fails, go ahead and git cl dcommit > > > > manually. > > > > > > Wait, please don't dcommit. The failures look related to your patch: > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/1519... > > > > Actually, I just noticed that there is a bug here... I forgot to rename one of > > the instances of domExpectedActivity2. About to submit a new patch. > > OK, now I don't see any test failures that look related to the patch, though > there do seem to be a bunch of unrelated browser_tests failing. Is that normal? > > In the cases I can see where the ActivityLogApiTest.TriggerEvent ran, it > succeeded. It is normal, but also sometimes challenging to tell the difference between failures caused by your patch or not. These failures don't look related to me, but to be sure you can just wait a while and then try the failing bot again (since in the interim someone likely will have fixed the flaky/failing tests).
Hi Tom, I'm back from vacation/conference now. What's the current status of this CL?
On 2013/07/29 17:12:58, felt wrote: > Hi Tom, I'm back from vacation/conference now. What's the current status of this > CL? I tried the build bots on Friday, and they all seemed to work except win_rel, though its failures seemed entirely unrelated. I just started win_rel again.
On 2013/07/29 17:16:03, tmroeder wrote: > On 2013/07/29 17:12:58, felt wrote: > > Hi Tom, I'm back from vacation/conference now. What's the current status of > this > > CL? > > I tried the build bots on Friday, and they all seemed to work except win_rel, > though its failures seemed entirely unrelated. I just started win_rel again. The win_rel failures all look completely unrelated to me. You can feel free to send it to the CQ now if you're comfortable with that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmroeder@chromium.org/19540019/56001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmroeder@chromium.org/19540019/56001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmroeder@chromium.org/19540019/56001
Message was sent while issue was closed.
Change committed as 214447 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
