Issue 29874649: Issue 6822 - Report browser test errors (Closed)
|
Created:
Sept. 4, 2018, 7:03 p.m. by hub Modified:
Sept. 21, 2018, 4:17 p.m. CC:
tlucas Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 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: 4
Patch Set 6 : Remove the finally() from the promise #Patch Set 7 : Properly ensure webdriver quit #Patch Set 8 : Reformating #Patch Set 9 : Fix error reporting #
Total comments: 2
Patch Set 10 : Wait on the driver.quit() promise #Patch Set 11 : eslint fixes #
Total comments: 15
Patch Set 12 : throw instead of Promise.reject() #
MessagesTotal messages: 21
I have tested with an actual failure in the browser test. Also if we move to Mocha, some of the things will probably need to be changed.
Sorry about the slightly superficial review so far, I'll dig deeper into this tomorrow. https://codereview.adblockplus.org/29874649/diff/29874650/test/browser/_boots... File test/browser/_bootstrap.js (right): https://codereview.adblockplus.org/29874649/diff/29874650/test/browser/_boots... test/browser/_bootstrap.js:49: if (typeof window.consoleLogs === "undefined") I have nothing against using strict equality where it's needed, but we don't use it by default in core and at least here it's definitely not needed (result of `typeof` is always a string), right? https://codereview.adblockplus.org/29874649/diff/29874650/test/runners/webdri... File test/runners/webdriver.js (right): https://codereview.adblockplus.org/29874649/diff/29874650/test/runners/webdri... test/runners/webdriver.js:37: result.log.forEach(item => console.log(item)); By the way, `console.log` works even when called without the correct `this`, so `result.log.forEach(console.log)` should work fine too; although they way you've written it is also easier to follow. It's your call. https://codereview.adblockplus.org/29874649/diff/29874650/test/runners/webdri... test/runners/webdriver.js:41: .finally(() => driver.quit()); Is `Promise.finally` already supported on the platform on which this code is going to run? It's a new feature, I didn't know it was available already. Just checking.
updated patch. https://codereview.adblockplus.org/29874649/diff/29874650/test/browser/_boots... File test/browser/_bootstrap.js (right): https://codereview.adblockplus.org/29874649/diff/29874650/test/browser/_boots... test/browser/_bootstrap.js:49: if (typeof window.consoleLogs === "undefined") On 2018/09/05 19:47:21, Manish Jethani wrote: > I have nothing against using strict equality where it's needed, but we don't use > it by default in core and at least here it's definitely not needed (result of > `typeof` is always a string), right? Done. https://codereview.adblockplus.org/29874649/diff/29874650/test/runners/webdri... File test/runners/webdriver.js (right): https://codereview.adblockplus.org/29874649/diff/29874650/test/runners/webdri... test/runners/webdriver.js:37: result.log.forEach(item => console.log(item)); On 2018/09/05 19:47:22, Manish Jethani wrote: > By the way, `console.log` works even when called without the correct `this`, so > `result.log.forEach(console.log)` should work fine too; although they way you've > written it is also easier to follow. It's your call. Acknowledged. https://codereview.adblockplus.org/29874649/diff/29874650/test/runners/webdri... test/runners/webdriver.js:41: .finally(() => driver.quit()); On 2018/09/05 19:47:21, Manish Jethani wrote: > Is `Promise.finally` already supported on the platform on which this code is > going to run? It's a new feature, I didn't know it was available already. Just > checking. excellent question. I can't find any mention the NodeJS documentation, while MDN say it is available in Node 10 only. I have tested with Node 7 and Node 8 and it works. So I assume it is ok since Node 7 is the minimum we require. I know also this contradict https://codereview.adblockplus.org/29860555/diff/29863569/test/firefox.js#new...
I've looked into this a little more today. First of all, from strictly the point of view of the Core module, only test/browser/_bootstrap.js should concern me, and the changes there look good to me. The rest is up to Sebastian. Now I have noticed a couple of things so far: - When the tests fail on Firefox, it says "Unit test "firefox" failed". Is there a way to include the name of the test that failed? - This doesn't seem to work for "chromium_remote". Is this how it's supposed to be?
On 2018/09/11 15:22:13, Manish Jethani wrote: > First of all, from strictly the point of view of the Core module, only > test/browser/_bootstrap.js should concern me, and the changes there look good to > me. The rest is up to Sebastian. Just to clarify this point, I don't know much about the test framework here; if it looks good to Sebastian then feel free to go ahead and land it.
On 2018/09/11 15:24:36, Manish Jethani wrote: > On 2018/09/11 15:22:13, Manish Jethani wrote: > > > First of all, from strictly the point of view of the Core module, only > > test/browser/_bootstrap.js should concern me, and the changes there look good > to > > me. The rest is up to Sebastian. > > Just to clarify this point, I don't know much about the test framework here; if > it looks good to Sebastian then feel free to go ahead and land it. Only one thing: We're updating the core dependency in the web extension, after that point we might want to branch off into a new next branch for future work. Please check with me about this before committing and pushing.
On 2018/09/11 15:22:13, Manish Jethani wrote: > - When the tests fail on Firefox, it says "Unit test "firefox" failed". Is > there a way to include the name of the test that failed? It shows the test results above. > > - This doesn't seem to work for "chromium_remote". Is this how it's supposed > to be? Damn. I'll make sure of this too.
On 2018/09/11 15:46:43, hub wrote: > > > > - This doesn't seem to work for "chromium_remote". Is this how it's supposed > > to be? > > Damn. I'll make sure of this too. It does now.
On 2018/09/13 20:49:50, hub wrote: > On 2018/09/11 15:46:43, hub wrote: > > > > > > > - This doesn't seem to work for "chromium_remote". Is this how it's > supposed > > > to be? > > > > Damn. I'll make sure of this too. > > It does now. Verified, works for me too. I'm OK with the changes in test/browser/ so at this point you can include Sebastian in the reviewers list so he can take a look at the rest of the changes.
https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_boots... File test/browser/_bootstrap.js (right): https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_boots... test/browser/_bootstrap.js:50: window.consoleLogs = {failures: 0, log: []}; This variable will be visible inside the tests, right? Maybe we should hide it somehow (at very least prefix it with underscored)? https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/chromi... File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/chromi... test/runners/chromium_remote_process.js:188: if (result.result.value.failures != 0) I'm not sure if I understand correctly, but wouldn't we want to log the failure messages themselves rather than just seeing "Test failure" if any browser error occurred? https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri... File test/runners/webdriver.js (left): https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri... test/runners/webdriver.js:29: console.log = msg => { What is about console.error(), console.warn(), etc.? Also what is about unhandled exceptions (potentially in asynchronous callbacks)? 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:37: result.log.forEach(item => console.log(item)); Nit: We generally prefer for..of loops of .forEach(). https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri... test/runners/webdriver.js:41: .finally(() => driver.quit()); .finally() requires Node.js 10, but we still support Node.js 7.
I haven't dealt yet with the `finally` on the Promise. https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_boots... File test/browser/_bootstrap.js (right): https://codereview.adblockplus.org/29874649/diff/29880043/test/browser/_boots... test/browser/_bootstrap.js:50: window.consoleLogs = {failures: 0, log: []}; On 2018/09/17 16:18:06, Sebastian Noack wrote: > This variable will be visible inside the tests, right? Maybe we should hide it > somehow (at very least prefix it with underscored)? Done. https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/chromi... File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/chromi... test/runners/chromium_remote_process.js:188: if (result.result.value.failures != 0) On 2018/09/17 16:18:06, Sebastian Noack wrote: > I'm not sure if I understand correctly, but wouldn't we want to log the failure > messages themselves rather than just seeing "Test failure" if any browser error > occurred? the messages ARE displayed aleready. Have always been. This is just for propagating the error back up. https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri... File test/runners/webdriver.js (left): https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri... test/runners/webdriver.js:29: console.log = msg => { On 2018/09/17 16:18:06, Sebastian Noack wrote: > What is about console.error(), console.warn(), etc.? > > Also what is about unhandled exceptions (potentially in asynchronous callbacks)? Only console.log is used for reporting test results. Exception are also caught by the framework. 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:37: result.log.forEach(item => console.log(item)); On 2018/09/17 16:18:06, Sebastian Noack wrote: > Nit: We generally prefer for..of loops of .forEach(). Done. https://codereview.adblockplus.org/29874649/diff/29880043/test/runners/webdri... test/runners/webdriver.js:41: .finally(() => driver.quit()); 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.
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?
still some problems with the browser instance left behind.
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)" On 2018/09/19 08:16:51, Sebastian Noack wrote: > Nit: Perhaps use a template string here. Done. 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"); On 2018/09/19 08:16:51, Sebastian Noack wrote: > In which case if the above promise rejected with null/undefined? Why? Fixed it.
https://codereview.adblockplus.org/29874649/diff/29885592/test/runners/webdri... File test/runners/webdriver.js (right): https://codereview.adblockplus.org/29874649/diff/29885592/test/runners/webdri... test/runners/webdriver.js:44: driver.quit(); When I was still using nodeunit in adblockpluschrome, I noticed that the Chromium process weren't properly terminated if I didn't wait for the promise returned by quit() to resolve.
Updated patch https://codereview.adblockplus.org/29874649/diff/29885592/test/runners/webdri... File test/runners/webdriver.js (right): https://codereview.adblockplus.org/29874649/diff/29885592/test/runners/webdri... test/runners/webdriver.js:44: driver.quit(); On 2018/09/20 17:34:00, Sebastian Noack wrote: > When I was still using nodeunit in adblockpluschrome, I noticed that the > Chromium process weren't properly terminated if I didn't wait for the promise > returned by quit() to resolve. I didn't, but this totally make sense. Fixing this.
https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/chromi... File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/chromi... test/runners/chromium_remote_process.js:189: return Promise.reject("Chromium (Remote Interface)"); Nit: You can just use throw here. https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... File test/runners/webdriver.js (right): https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... test/runners/webdriver.js:42: return Promise.reject(name); Nit: You can just use throw here. https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... test/runners/webdriver.js:46: return driver.quit().then(() => Promise.reject(error)); Please leave a comment that this code should be replaced with .finally(() => driver.quit()) once we require Node.js >= 10. https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... test/runners/webdriver.js:46: return driver.quit().then(() => Promise.reject(error)); Now, we call driver.quit() twice if there are any failures. Does that work, or would that cause a new promise rejection which silences the actual error? https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js#newc... test_runner.js:158: // or the test will not let close the webdriver. Does that still hold true with the changes to test/runners/webdriver.js where we now make sure the driver is properly closed on failure? https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js#newc... test_runner.js:164: return Promise.reject(`Browser unit test failed: ${errors.join(", ")}`); Nit: You can just use throw here. https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js#newc... test_runner.js:186: process.exit(1); What would happen if we just leave this error unhandled?
updated patch https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/chromi... File test/runners/chromium_remote_process.js (right): https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/chromi... test/runners/chromium_remote_process.js:189: return Promise.reject("Chromium (Remote Interface)"); On 2018/09/21 14:22:20, Sebastian Noack wrote: > Nit: You can just use throw here. Done. https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... File test/runners/webdriver.js (right): https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... test/runners/webdriver.js:42: return Promise.reject(name); On 2018/09/21 14:22:21, Sebastian Noack wrote: > Nit: You can just use throw here. Done. https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... test/runners/webdriver.js:46: return driver.quit().then(() => Promise.reject(error)); On 2018/09/21 14:22:21, Sebastian Noack wrote: > Now, we call driver.quit() twice if there are any failures. No we don't. We are on the rejection handler for that `then`. So we don't reach here if `result.failures != 0` > Does that work, or > would that cause a new promise rejection which silences the actual error? Here I pass the actual error if `driver.quit()` is successful. Otherwise we get the `driver.quit()` error. Note that to confirm the answer above I'd get an error if I call `driver.quit()` twice. https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... test/runners/webdriver.js:46: return driver.quit().then(() => Promise.reject(error)); On 2018/09/21 14:22:20, Sebastian Noack wrote: > Please leave a comment that this code should be replaced with > .finally(() => driver.quit()) once we require Node.js >= 10. Done. https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js#newc... test_runner.js:158: // or the test will not let close the webdriver. On 2018/09/21 14:22:21, Sebastian Noack wrote: > Does that still hold true with the changes to test/runners/webdriver.js where we > now make sure the driver is properly closed on failure? Yes. The problem is that as soon as one of the promise is rejected in `Promise.all` we continue past, and the the script terminates which doesn't leave the time for the browser instances to be quit. https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js#newc... test_runner.js:164: return Promise.reject(`Browser unit test failed: ${errors.join(", ")}`); On 2018/09/21 14:22:21, Sebastian Noack wrote: > Nit: You can just use throw here. Done. https://codereview.adblockplus.org/29874649/diff/29886703/test_runner.js#newc... test_runner.js:186: process.exit(1); On 2018/09/21 14:22:21, Sebastian Noack wrote: > What would happen if we just leave this error unhandled? Currently you get a warning about unhandled promise rejection. On more recent Nodejs you are supposed to get a non-zero exit code with termination, at least that's what the deprecation warning says.
LGTM. Note that the two comments I didn't reply to wouldn't be an issue with Mocha, where test cases can just return a promise, and it takes care to report any promise rejection as an error causing the test fail. Also you can just close the driver in an after() or afterEach() hook, which can return a promise as well, rather than the dance we do here to make sure the driver is destroyed. Considering the above and the fact that nodeunit is deprecated, we should really consider migrating this test suite to Mocha (like we already did in adblockpluscore). https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... File test/runners/webdriver.js (right): https://codereview.adblockplus.org/29874649/diff/29886703/test/runners/webdri... test/runners/webdriver.js:46: return driver.quit().then(() => Promise.reject(error)); On 2018/09/21 15:39:35, hub wrote: > On 2018/09/21 14:22:21, Sebastian Noack wrote: > > Now, we call driver.quit() twice if there are any failures. > > No we don't. We are on the rejection handler for that `then`. So we don't reach > here if `result.failures != 0` > > > Does that work, or > > would that cause a new promise rejection which silences the actual error? > > Here I pass the actual error if `driver.quit()` is successful. Otherwise we get > the `driver.quit()` error. > Note that to confirm the answer above I'd get an error if I call `driver.quit()` > twice. You are right. I misread the code, I thought we are in another catch callback here. FWIW, if I have more than one callback, I prefer to wrap the code like this, which makes that more obvious: .then( result => { }, error => { } )
On 2018/09/21 15:58:46, Sebastian Noack wrote: > LGTM. > > Note that the two comments I didn't reply to wouldn't be an issue with Mocha, > where test cases can just return a promise, and it takes care to report any > promise rejection as an error causing the test fail. Also you can just close the > driver in an after() or afterEach() hook, which can return a promise as well, > rather than the dance we do here to make sure the driver is destroyed. > > Considering the above and the fact that nodeunit is deprecated, we should really > consider migrating this test suite to Mocha (like we already did in > adblockpluscore). > I already filed https://issues.adblockplus.org/ticket/6820 a while back. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
