Issue 29622595: Issue 6090 - Properly use regexp for message matching and proper console override (Closed)
|
Created:
Nov. 28, 2017, 3:28 p.m. by hub Modified:
Nov. 29, 2017, 8:02 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6090 - Properly use regexp for message matching and proper console override
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix a syntax error. #
Total comments: 5
Patch Set 3 : Properly handle non-string arguments for console.*() #
Total comments: 9
Patch Set 4 : Properly handle args #MessagesTotal messages: 12
https://codereview.adblockplus.org/29622595/diff/29622596/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622596/test/_common.js#new... test/_common.js:440: console.error = s => should not we wrap even a single argument of the lambdas (also above) in parentheses? `(s) => ...` https://codereview.adblockplus.org/29622595/diff/29622596/test/_common.js#new... test/_common.js:442: let match = msgMatch.match(s); AFAIR regexp does not have method `match`, it should be either msgMatch.exec or s.match(msgMatch), IMO the former is better.
https://codereview.adblockplus.org/29622595/diff/29622596/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622596/test/_common.js#new... test/_common.js:440: console.error = s => On 2017/11/28 15:46:22, sergei wrote: > should not we wrap even a single argument of the lambdas (also above) in > parentheses? `(s) => ...` This follow our code style. https://codereview.adblockplus.org/29622595/diff/29622596/test/_common.js#new... test/_common.js:442: let match = msgMatch.match(s); On 2017/11/28 15:46:22, sergei wrote: > AFAIR regexp does not have method `match`, it should be either msgMatch.exec or > s.match(msgMatch), IMO the former is better. Already fixed in the patch update. I have no idea how I didn't catch it prior to submission :-/
https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js#new... test/_common.js:438: let msgMatch = new RegExp("^Error: (.*)[\r\n]"); Another alternative is: let msgMatch = new RegExp("^Error: (.*)$", "m"); But then we are no longer matching the first line, just any line.
https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js#new... test/_common.js:438: let msgMatch = new RegExp("^Error: (.*)[\r\n]"); So, what about looking at s.message only? https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js#new... test/_common.js:440: console.error = s => Nit: Why is this parameter called s? It's not necessarily a string, typically it will be an Error object. Call it err maybe?
LGTM
https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js#new... test/_common.js:440: console.error = s => On 2017/11/28 20:00:57, Wladimir Palant wrote: > Nit: Why is this parameter called s? It's not necessarily a string, typically it > will be an Error object. Call it err maybe? It's Error.prototype.stack (https://github.com/adblockplus/adblockpluscore/blob/4939e6a861dba5a9327829629...) what is string. I would like to add that in general it would be better to interrupt a JS function corresponding to C++ `assert` instead of console.error, but there is no such function, at least so far. In that function it would be fair to expect an Error object or any another special object, but since it's expected that console.error uses string representation of its arguments, strictly speaking we should not try to dig the nature of arguments and to treat them differently in order to avoid a potential side effect, though it's pure theoretical. BTW, that was one of the reasons that I mentioned that it would be better to use msgMatch.exec(s) than `s.match(msgMatch)` because the latter can result in anything. So, if you decide to make changes here to expect an Error object then I would ask to add JS `assert` function and silence an expected output only for that function.
https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622604/test/_common.js#new... test/_common.js:440: console.error = s => On 2017/11/28 20:00:57, Wladimir Palant wrote: > Nit: Why is this parameter called s? It's not necessarily a string, typically it > will be an Error object. Call it err maybe? Oh. I see. In our code, assert2() will always call console.error() with a string as a parameter. But in general it could be call with any object and any parameters. And that would lead to a failure. But we don't go that route. I'll update the patch to make sure we deal properly with the situation.
https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:424: if (typeof args[0] === "string") To be consistent with other JS code it should not be a strict comparison, just `==`. https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:427: s = args[0].message; args[0] can be undefined and before accessing `message` property we should check that it's an instance of Error. However, IMO it's a dead code and overengineering at that moment, here and below. https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:430: warnHandler(args); strictly speaking it should be warnHandler.warn.apply(console, args)
https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:427: s = args[0].message; On 2017/11/29 09:31:54, sergei wrote: > args[0] can be undefined and before accessing `message` property we should check > that it's an instance of Error. Yes, the better approach is: let s = (args[0] instanceof Error ? args[0].message : args[0]); Same thing below. https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:430: warnHandler(args); This should be `warnHandler(...args)` - pass the original parameters as parameters, not as an array. Same with errorHandler below. https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:455: if (!match || match[1] != msg) So why still use the regular expression if you are looking a the message only? It can be a normal string comparison now, same as with silenceWarnOutput. if (s != msg)
https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js File test/_common.js (right): https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:427: s = args[0].message; On 2017/11/29 10:27:31, Wladimir Palant wrote: > On 2017/11/29 09:31:54, sergei wrote: > > args[0] can be undefined and before accessing `message` property we should > check > > that it's an instance of Error. > > Yes, the better approach is: > > let s = (args[0] instanceof Error ? args[0].message : args[0]); > > Same thing below. Done. https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:430: warnHandler(args); On 2017/11/29 10:27:31, Wladimir Palant wrote: > This should be `warnHandler(...args)` - pass the original parameters as > parameters, not as an array. Same with errorHandler below. Done. https://codereview.adblockplus.org/29622595/diff/29622617/test/_common.js#new... test/_common.js:455: if (!match || match[1] != msg) On 2017/11/29 10:27:31, Wladimir Palant wrote: > So why still use the regular expression if you are looking a the message only? > It can be a normal string comparison now, same as with silenceWarnOutput. > > if (s != msg) We want to silence an assert (call to our assert2()) so we do get as the error message the whole stack trace. While I could put "Error: " part of the message instead of the matching Regexp. I still need to match and not equal. Other option include susbtr(), split the lines and equality on the first one. This also mean that here we are unlikely to ever get a something that is not a string as the only argument.
LGTM 
|
