Issue 29616611: Issue 6090 - Allow use of console to be seen during tests (Closed)
|
Created:
Nov. 23, 2017, 7:19 p.m. by hub Modified:
Nov. 28, 2017, 3:29 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6090 - Allow use of console to be seen during tests
Patch Set 1 #
Total comments: 12
Patch Set 2 : Added missing patch. Review feedback #
Total comments: 2
Patch Set 3 : review feedback #
Total comments: 4
MessagesTotal messages: 12
this patch allow: 1. to have console output from compiled code to be displayed. This include printf(), LogError(), LogString(), assert2(), etc. 2. to have console silenced for a specific error using maybeExpectError() The need of this arose with sergei patch that properly build in debug https://codereview.adblockplus.org/29613611/ . With this change we can actually catch problems like the one fixed by https://codereview.adblockplus.org/29616589/
IMO it's a serious question which requires some background information, therefore I have created the issue for it https://issues.adblockplus.org/ticket/6090, please change the review description accordingly. PS: it seems the base commit is missing, the corresponding changes are also added into the issue's todo. https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:422: exports.maybeExpectError = function(msg, test) What about calling it `silenceAssertionOutput`? https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:425: let errorHandler = globals.console.error; I think it's OK taking into account that `console` is bound to `console.error` and it's even in the same file. https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:428: if (!msgMatch.test(s) && typeof errorHandler === "function") I think we should not test the type of errorHandler because it would be better to crash if it's not a function. https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:433: test(); IMO we should return the result of `test()` for the sake of simplicity. https://codereview.adblockplus.org/29616611/diff/29616612/test/filterStorage.js File test/filterStorage.js (right): https://codereview.adblockplus.org/29616611/diff/29616612/test/filterStorage.... test/filterStorage.js:189: () => test.ok(!FilterStorage.moveSubscription(subscription2), "Move of removed subscription failed") What about being more strict and wrap only the call of `FilterStorage.moveSubscription(subscription2)`? test.ok(!maybeExpectError(() => FilterStorage.moveSubscription(subscription2), "Attempt to move a subscription that is not in the list"), "Move of removed subscription failed");
https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:422: exports.maybeExpectError = function(msg, test) On 2017/11/24 10:15:28, sergei wrote: > What about calling it `silenceAssertionOutput`? JIC, I also thought that we could want to actually test that there is such output from time to time by enabling an additional flag, and in this case something like expectAssertionOutput would make sense, but I doubt that we should really test assertion messages. We still can do it technically but basically we want to deliberately not show expected messages.
https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:422: exports.maybeExpectError = function(msg, test) On 2017/11/24 13:49:13, sergei wrote: > On 2017/11/24 10:15:28, sergei wrote: > > What about calling it `silenceAssertionOutput`? > > JIC, I also thought that we could want to actually test that there is such > output from time to time by enabling an additional flag, and in this case > something like expectAssertionOutput would make sense, but I doubt that we > should really test assertion messages. We still can do it technically but > basically we want to deliberately not show expected messages. I thought about expecting the output but then I'd need to check this is a DEBUG build, otherwise assert2() is a noop. https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:422: exports.maybeExpectError = function(msg, test) On 2017/11/24 10:15:28, sergei wrote: > What about calling it `silenceAssertionOutput`? Done. https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:425: let errorHandler = globals.console.error; On 2017/11/24 10:15:28, sergei wrote: > I think it's OK taking into account that `console` is bound to `console.error` > and it's even in the same file. I'm missing a patch here where we actualle make console.log and console.error work, I'll update with it. https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:428: if (!msgMatch.test(s) && typeof errorHandler === "function") On 2017/11/24 10:15:28, sergei wrote: > I think we should not test the type of errorHandler because it would be better > to crash if it's not a function. Acknowledged. https://codereview.adblockplus.org/29616611/diff/29616612/test/_common.js#new... test/_common.js:433: test(); On 2017/11/24 10:15:28, sergei wrote: > IMO we should return the result of `test()` for the sake of simplicity. Acknowledged. https://codereview.adblockplus.org/29616611/diff/29616612/test/filterStorage.js File test/filterStorage.js (right): https://codereview.adblockplus.org/29616611/diff/29616612/test/filterStorage.... test/filterStorage.js:189: () => test.ok(!FilterStorage.moveSubscription(subscription2), "Move of removed subscription failed") On 2017/11/24 10:15:28, sergei wrote: > What about being more strict and wrap only the call of > `FilterStorage.moveSubscription(subscription2)`? > > test.ok(!maybeExpectError(() => FilterStorage.moveSubscription(subscription2), > "Attempt to move a subscription that is not in the > list"), > "Move of removed subscription failed"); Done.
LGTM
https://codereview.adblockplus.org/29616611/diff/29617571/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29616611/diff/29617571/test/_common.js#new... test/_common.js:434: result = test(); There is no necessity in an additional variable, could you please just return from the try-block?
https://codereview.adblockplus.org/29616611/diff/29617571/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29616611/diff/29617571/test/_common.js#new... test/_common.js:434: result = test(); On 2017/11/24 16:46:27, sergei wrote: > There is no necessity in an additional variable, could you please just return > from the try-block? For some reason I believed that return would by-pass the finally.... Done
LGTM, however test/signatures.js also prints some messages but it uses console.warn. Since it's not exactly C++ assertions could you please address it but it in another review/commit? Maybe just add some similar silenceWarnOutput(fn, msg).
but without https://codereview.adblockplus.org/29616589/ the output is gonna be very verbose.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29616611/diff/29617577/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29616611/diff/29617577/test/_common.js#new... test/_common.js:97: }, Just remove console here, not overriding this global will do. https://codereview.adblockplus.org/29616611/diff/29617577/test/_common.js#new... test/_common.js:424: let msgMatch = new RegExp(`^Error: ${msg}[\r\n]`); 1) Inserting unescaped strings into a regular expression is a footgun (consider dots or question marks). If anything, the regular expression should be used to extract the message, then you should use regular string comparison. 2) Parsing string representation of errors isn't a good idea either, it can change. You have an Error object here, just look at the `error.message` property.
Message was sent while issue was closed.
Follow up patch at https://codereview.adblockplus.org/29622595/ https://codereview.adblockplus.org/29616611/diff/29617577/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29616611/diff/29617577/test/_common.js#new... test/_common.js:97: }, On 2017/11/28 12:59:39, Wladimir Palant wrote: > Just remove console here, not overriding this global will do. Done in follow up patch https://codereview.adblockplus.org/29616611/diff/29617577/test/_common.js#new... test/_common.js:424: let msgMatch = new RegExp(`^Error: ${msg}[\r\n]`); On 2017/11/28 12:59:40, Wladimir Palant wrote: > 1) Inserting unescaped strings into a regular expression is a footgun (consider > dots or question marks). If anything, the regular expression should be used to > extract the message, then you should use regular string comparison. Done in followup patch. > 2) Parsing string representation of errors isn't a good idea either, it can > change. You have an Error object here, just look at the `error.message` > property. we only have a string at that level. We'd need to change the error reporting. Since this is only in the test, I consider it to be a reasonable assumption. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
