Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(567)

Issue 29874649: Issue 6822 - Report browser test errors

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks ago by hub
Modified:
10 hours, 10 minutes ago
CC:
tlucas
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6822 - Report browser test errors

Patch Set 1 #

Total comments: 6

Patch Set 2 : Updated patch #

Patch Set 3 : Also report from the chrome-remote-interface #

Patch Set 4 : Minor reformat #

Total comments: 11

Patch Set 5 : Updated patch #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -13 lines) Patch
M test/browser/_bootstrap.js View 1 2 3 4 2 chunks +4 lines, -1 line 1 comment Download
M test/runners/chromium_process.js View 1 chunk +1 line, -0 lines 0 comments Download
M test/runners/chromium_remote_process.js View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M test/runners/webdriver.js View 1 2 3 4 1 chunk +8 lines, -7 lines 0 comments Download
M test_runner.js View 1 chunk +10 lines, -5 lines 1 comment Download

Messages

Total messages: 13
hub
2 weeks ago (2018-09-04 19:03:25 UTC) #1
hub
I have tested with an actual failure in the browser test. Also if we move ...
2 weeks ago (2018-09-04 19:04:56 UTC) #2
Manish Jethani
Sorry about the slightly superficial review so far, I'll dig deeper into this tomorrow. https://codereview.adblockplus.org/29874649/diff/29874650/test/browser/_bootstrap.js ...
1 week, 6 days ago (2018-09-05 19:47:22 UTC) #3
hub
updated patch. https://codereview.adblockplus.org/29874649/diff/29874650/test/browser/_bootstrap.js File test/browser/_bootstrap.js (right): https://codereview.adblockplus.org/29874649/diff/29874650/test/browser/_bootstrap.js#newcode49 test/browser/_bootstrap.js:49: if (typeof window.consoleLogs === "undefined") On 2018/09/05 ...
1 week, 6 days ago (2018-09-06 01:00:13 UTC) #4
Manish Jethani
I've looked into this a little more today. First of all, from strictly the point ...
1 week, 1 day ago (2018-09-11 15:22:13 UTC) #5
Manish Jethani
On 2018/09/11 15:22:13, Manish Jethani wrote: > First of all, from strictly the point of ...
1 week, 1 day ago (2018-09-11 15:24:36 UTC) #6
Manish Jethani
On 2018/09/11 15:24:36, Manish Jethani wrote: > On 2018/09/11 15:22:13, Manish Jethani wrote: > > ...
1 week, 1 day ago (2018-09-11 15:31:05 UTC) #7
hub
On 2018/09/11 15:22:13, Manish Jethani wrote: > - When the tests fail on Firefox, it ...
1 week, 1 day ago (2018-09-11 15:46:43 UTC) #8
hub
On 2018/09/11 15:46:43, hub wrote: > > > > - This doesn't seem to work ...
5 days, 21 hours ago (2018-09-13 20:49:50 UTC) #9
Manish Jethani
On 2018/09/13 20:49:50, hub wrote: > On 2018/09/11 15:46:43, hub wrote: > > > > ...
5 days, 13 hours ago (2018-09-14 04:50:49 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_bootstrap.js File test/browser/_bootstrap.js (right): https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_bootstrap.js#newcode50 test/browser/_bootstrap.js:50: window.consoleLogs = {failures: 0, log: []}; This variable will ...
2 days, 2 hours ago (2018-09-17 16:18:06 UTC) #11
hub
I haven't dealt yet with the `finally` on the Promise. https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_bootstrap.js File test/browser/_bootstrap.js (right): https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_bootstrap.js#newcode50 ...
20 hours, 3 minutes ago (2018-09-18 22:23:37 UTC) #12
Sebastian Noack
10 hours, 10 minutes ago (2018-09-19 08:16:51 UTC) #13
https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri...
File test/runners/webdriver.js (right):

https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri...
test/runners/webdriver.js:41: .finally(() => driver.quit());
On 2018/09/18 22:23:36, hub wrote:
> On 2018/09/17 16:18:06, Sebastian Noack wrote:
> > .finally() requires Node.js 10, but we still support Node.js 7.
> 
> I have run this on Node 7 and 8 and it did work. I am not sure why though.
Will
> fix this it.

I'm on Node.js 8.5.0 and Promise.resolve().finally gives me undefined. If this
code doesn't error out, then I guess it is not executed.

https://codereview.adblockplus.org/29874649/diff/29884590/test/browser/_boots...
File test/browser/_bootstrap.js (right):

https://codereview.adblockplus.org/29874649/diff/29884590/test/browser/_boots...
test/browser/_bootstrap.js:89: assertions.length + " assertions (" +
assertions.duration + "ms)"
Nit: Perhaps use a template string here.

https://codereview.adblockplus.org/29874649/diff/29884590/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29874649/diff/29884590/test_runner.js#newc...
test_runner.js:181: console.error("Failed running browser tests");
In which case if the above promise rejected with null/undefined? Why?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5