Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7250009:
Added guts to pull call signatures when assertions & expectations fail. (Closed)
|
Created:
9 years, 5 months ago by Sheridan Rawlins Modified:
9 years, 5 months ago CC:
chromium-reviews, Paweł Hajdan Jr., Lei Zhang, Evan Stade Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded guts to pull call signatures when assertions & expectations fail.
Added sample_fail.js to show failing messages & allow generation of ASSERT_FALSE.
BUG=89349
TEST=browser_tests --gtest_filter=WebUIAssertionsTest.*
R=phajdan.jr@chromium.org, dtseng@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93769
Patch Set 1 #
Total comments: 29
Patch Set 2 : Rebase, fix and fix nits. #
Total comments: 19
Patch Set 3 : Changes from review. #
Total comments: 2
Patch Set 4 : Use Regex instead of parenthesis-counting. #Patch Set 5 : Move GEN lines to assertions-inl.h #Patch Set 6 : Added copyright header. #Patch Set 7 : Include constructed messages instead of just call signature. #
Total comments: 1
Patch Set 8 : Fix clang errors. #Patch Set 9 : Moved definition of constructor/destructor into .cc for clang. #Patch Set 10 : clang #
Messages
Total messages: 21 (0 generated)
Pawel - here's the better messaging on failures, which you asked about in a previous CL. FYI, the matching code is only exercised on failures to generate the message, otherwise the registry just bumps the count of the dictionary entry for the caller and the assert/except name (in case there are several calls to the same thing like assertTrue). This also handles calling helper functions which do the actual calls to assert/expect methods by wrapping around. Since the index starts at 0, and the count starts at 1, we have to do some magic to wrap around which is ((count - 1) % index) + 1, which basically normalizes the count to starting at zero which is correct for the mod (%), then puts it back to starting at 1. Also, to support ASSERT_FALSE, it it now possible to extend the generator as needed, and for this case sample_fail.js sets var test_assert = 'ASSERT_FALSE'; which tells the generator to use ASSERT_FALSE rather than the default ASSERT_TRUE around the call to RunJavascriptTest.
http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... File chrome/browser/ui/webui/javascript2webui.js (left): http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:6: arguments[0] + ' path-to-testfile.js testfile.js [output.cc]'); Nit: is this really the usage? How do I run this using v8 shell? http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:18: if (!('test_fixture' in this)) { is this a string? Should we check to make sure? http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:29: print(' AddLibrary(FilePath(FILE_PATH_LITERAL("' + js_file_base + '")));'); Doesn't this belong in SetUpTestFixture? http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... File chrome/browser/ui/webui/javascript2webui.js (right): http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:9: var js_file = arguments[1]; Contradicts usage message above path-to-testfile.js testfile.js http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:10: var js_file_base = arguments[2]; This contradicts your usage message above: path-to-testfile.js testfile.js . Perhaps name it library? http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:23: this['test_assert'] = 'ASSERT_TRUE'; Why don't we just make this a boolean and resolve to the right macro here? The test js file would just say true/false for tests passing/failing. Also, this is scoped to the entire file. You can't selectively say that test1 ASSERT_TRUE(RunJavaScriptTest(foo) test2 ASSERT_FALSE(RunJavaScriptTest(bar) within the same js file.
http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api.js File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:47: var parencount = 1; why not 0? http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:50: parencount && index < string.length; parenCount http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:61: args.push(string.substring(arg_start, index - 1)); confused...this is outside of the loop? http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:69: if (caller_caller['isExpect']) { Would prefer if we didn't special case this (see below)... http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:71: caller_caller = caller_caller.caller; Would prefer if we just printed the whole stack trace to avoid doing stuff like this and might simplify this whole block. http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:91: search_start += caller_name.length; searchStart http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:99: 'getCall': function() { Why don't we just generate the whole stack trace? http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:169: expect_func.__proto__ = Function.prototype; expectFunc (or expectHelper). http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:171: expect_func.isExpect = true; Why don't we just subclass TOString to print out this object which would avoid having to check for this flag?
http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... File chrome/browser/ui/webui/javascript2webui.js (left): http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:18: if (!('test_fixture' in this)) { It should be a constructor. I suppose we could check its signature, but I'm going to roll back changes to this file. On 2011/07/08 22:37:18, David Tseng wrote: > is this a string? Should we check to make sure? http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:29: print(' AddLibrary(FilePath(FILE_PATH_LITERAL("' + js_file_base + '")));'); Yeah, this was overhauled in another patch. On 2011/07/08 22:37:18, David Tseng wrote: > Doesn't this belong in SetUpTestFixture? http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... File chrome/browser/ui/webui/javascript2webui.js (right): http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:9: var js_file = arguments[1]; Why? argv[0] is always the program name. On 2011/07/08 22:37:18, David Tseng wrote: > Contradicts usage message above > path-to-testfile.js testfile.js http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:10: var js_file_base = arguments[2]; Can you please describe the conflict? On 2011/07/08 22:37:18, David Tseng wrote: > This contradicts your usage message above: > path-to-testfile.js testfile.js . > > Perhaps name it library? http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api.js File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:47: var parencount = 1; Because you found the first paren, hence 1. On 2011/07/08 23:45:11, David Tseng wrote: > why not 0? http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:50: parencount && index < string.length; On 2011/07/08 23:45:11, David Tseng wrote: > parenCount Done. http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:61: args.push(string.substring(arg_start, index - 1)); On 2011/07/08 23:45:11, David Tseng wrote: > confused...this is outside of the loop? Moved inside the loop. http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:71: caller_caller = caller_caller.caller; We're trying to single out the text of the call. We already do include a stack trace and it doesn't help when there are two similar calls: TEST_F('foo','bar', function() { expectTrue(5 < 2) expectTrue(3 < 2) }); The stacktrace would only reference foo.bar and not have any indication of whether 3<2 failed or 5<2 failed. On 2011/07/08 23:45:11, David Tseng wrote: > Would prefer if we just printed the whole stack trace to avoid doing stuff like > this and might simplify this whole block. http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:91: search_start += caller_name.length; On 2011/07/08 23:45:11, David Tseng wrote: > searchStart Done. http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:99: 'getCall': function() { we do (it's in the Error and returned to C++) On 2011/07/08 23:45:11, David Tseng wrote: > Why don't we just generate the whole stack trace? http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:169: expect_func.__proto__ = Function.prototype; On 2011/07/08 23:45:11, David Tseng wrote: > expectFunc (or expectHelper). Done. http://codereview.chromium.org/7250009/diff/1/chrome/test/data/webui/test_api... chrome/test/data/webui/test_api.js:171: expect_func.isExpect = true; We would still need to do the replace somewhere - I don't think it helps to move it to tostring, especially since we need to know to adjust the stack up a level in that case (so we get the caller.caller) On 2011/07/08 23:45:11, David Tseng wrote: > Why don't we just subclass TOString to print out this object which would avoid > having to check for this flag?
> > http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... > chrome/browser/ui/webui/javascript2webui.js:18: if (!('test_fixture' in > this)) { > It should be a constructor. I suppose we could check its signature, but > I'm going to roll back changes to this file. I'll wait for you to ping when it's updated then. Thanks.
That file shouldn't be changed at all in Patch Set 2. -SCR On 2011/07/15 20:44:02, David Tseng wrote: > > > > > http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... > > chrome/browser/ui/webui/javascript2webui.js:18: if (!('test_fixture' in > > this)) { > > It should be a constructor. I suppose we could check its signature, but > > I'm going to roll back changes to this file. > > I'll wait for you to ping when it's updated then. Thanks.
http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... File chrome/browser/ui/webui/javascript2webui.js (right): http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:9: var js_file = arguments[1]; It's not clear from the usage message what the params are. i.e. path-to-testfile.js vs testfile.js what's the difference? Is one meant to be a path? Also, v8 shell's binary is always called "shell" isn't it? We should probably mention that this js file is meant to be executed with the V8 shell and provide a link since this is our only documentation. On 2011/07/14 23:32:31, Sheridan Rawlins wrote: > Why? argv[0] is always the program name. > > On 2011/07/08 22:37:18, David Tseng wrote: > > Contradicts usage message above > > path-to-testfile.js testfile.js >
http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... File chrome/browser/ui/webui/javascript2webui.js (right): http://codereview.chromium.org/7250009/diff/1/chrome/browser/ui/webui/javascr... chrome/browser/ui/webui/javascript2webui.js:9: var js_file = arguments[1]; As we discussed, I rolled this file back. I filed http://crbug.com/89613 to clean-up documentation/comment-generation. -SCR On 2011/07/18 16:41:37, David Tseng wrote: > It's not clear from the usage message what the params are. > > i.e. > path-to-testfile.js vs > testfile.js > > what's the difference? Is one meant to be a path? > > Also, v8 shell's binary is always called "shell" isn't it? We should probably > mention that this js file is meant to be executed with the V8 shell and provide > a link since this is our only documentation. > On 2011/07/14 23:32:31, Sheridan Rawlins wrote: > > Why? argv[0] is always the program name. > > > > On 2011/07/08 22:37:18, David Tseng wrote: > > > Contradicts usage message above > > > path-to-testfile.js testfile.js > > >
http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (left): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:77: * generation inside the function, and before any generated C++. nit: after? http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:156: this.body.call(this.fixture); Dnit: Does the |body| expect |fixture| to be non-null? http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:220: function CallHelper() { Document. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:225: 'counts_': {}, nit: should document this since it's not obvious you're using this to map the function signatures to a count. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:226: 'getStackInfo': function(caller) { nit: clearer to call this 'getCallerInfo", or 'getCallerSignature'? http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:244: this.counts_[stackInfo.callerCallerString][stackInfo.callerName] = 0; could we just combine the two strings stackInfo.callerCallerString, stackInfo.callerName as the key to count map? Perhaps add a "key" property to the "stackInfo" object? Then, we could combine the two blocks above. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:247: 'matchedParamsIndex': function(string, index) { nit: up to you, but it seems like we can do this much much more easily with String.match and a regex.
http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (left): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:77: * generation inside the function, and before any generated C++. On 2011/07/18 18:31:59, David Tseng wrote: > nit: after? Done. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:156: this.body.call(this.fixture); It doesn't need to, but it may, if you are writing TEST_F cases. For TEST cases, the implementation _may_ be null or undefined and still run fine as long as the body doesn't depend upon this - test author should know. I added a better comment. On 2011/07/18 18:31:59, David Tseng wrote: > Dnit: Does the |body| expect |fixture| to be non-null? http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:220: function CallHelper() { On 2011/07/18 18:31:59, David Tseng wrote: > Document. Done. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:225: 'counts_': {}, On 2011/07/18 18:31:59, David Tseng wrote: > nit: should document this since it's not obvious you're using this to map the > function signatures to a count. Done. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:226: 'getStackInfo': function(caller) { used getCallerInfo. Done. On 2011/07/18 18:31:59, David Tseng wrote: > nit: clearer to call this 'getCallerInfo", or 'getCallerSignature'? http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:244: this.counts_[stackInfo.callerCallerString][stackInfo.callerName] = 0; I could, but then we'd need to use a separator character (which could be ''). I don't like it as much as just indexing on two variables - it is really a 2-dimensional array, so why not code it as such? On 2011/07/18 18:31:59, David Tseng wrote: > could we just combine the two strings > stackInfo.callerCallerString, stackInfo.callerName > > as the key to count map? Perhaps add a "key" property to the "stackInfo" > object? Then, we could combine the two blocks above. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:247: 'matchedParamsIndex': function(string, index) { Hah, that's what you'd think. Unfortunately, it's not true - read the following link for more info. http://www.texttoolkit.com/index.php?option=com_content&view=article&id=63:pa... On 2011/07/18 18:31:59, David Tseng wrote: > nit: up to you, but it seems like we can do this much much more easily with > String.match and a regex.
LGTM; Pawel should still take a look. http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:247: 'matchedParamsIndex': function(string, index) { True, but are we not just looking for strings of the pattern foo(...); i.e. functionName + "(" + "\s\S" + ");" Also, the article correctly prints out that this is hard to do with a true language semantic parser because of cases like: foo("I'm a string with ( a paren!"); On 2011/07/19 01:19:25, Sheridan Rawlins wrote: > Hah, that's what you'd think. Unfortunately, it's not true - read the following > link for more info. > > http://www.texttoolkit.com/index.php?option=com_content&view=article&id=63:pa... > > On 2011/07/18 18:31:59, David Tseng wrote: > > nit: up to you, but it seems like we can do this much much more easily with > > String.match and a regex. >
http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:247: 'matchedParamsIndex': function(string, index) { What about assertTrue(expected, function(foo, bar))? I also wouldn't want to come back and debug if someone did something like you mentioned: assertTrue("(", getParen()); -SCR On 2011/07/19 16:53:03, David Tseng wrote: > True, but are we not just looking for strings of the pattern > foo(...); > i.e. > functionName + "(" + "\s\S" + ");" > > Also, the article correctly prints out that this is hard to do with a true > language semantic parser because of cases like: > foo("I'm a string with ( a paren!"); > On 2011/07/19 01:19:25, Sheridan Rawlins wrote: > > Hah, that's what you'd think. Unfortunately, it's not true - read the > following > > link for more info. > > > > > http://www.texttoolkit.com/index.php?option=com_content&view=article&id=63:pa... > > > > On 2011/07/18 18:31:59, David Tseng wrote: > > > nit: up to you, but it seems like we can do this much much more easily with > > > String.match and a regex. > > >
http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:247: 'matchedParamsIndex': function(string, index) { > Also, the article correctly prints out that this is hard to do without a true > language semantic parser because of cases like: > foo("I'm a string with ( a paren!"); I don't think the code as is handles this case. > On 2011/07/19 01:19:25, Sheridan Rawlins wrote: > > Hah, that's what you'd think. Unfortunately, it's not true - read the > following > > link for more info. > > > > > http://www.texttoolkit.com/index.php?option=com_content&view=article&id=63:pa... > > > > On 2011/07/18 18:31:59, David Tseng wrote: > > > nit: up to you, but it seems like we can do this much much more easily with > > > String.match and a regex. > > >
http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:247: 'matchedParamsIndex': function(string, index) { On 2011/07/19 16:57:40, Sheridan Rawlins wrote: > What about > assertTrue(expected, function(foo, bar))? You can make reg ex's lazy by using "?". Also note that we're matching on ");" and not just ")" for any reg ex's. > > I also wouldn't want to come back and debug if someone did something like you > mentioned: > assertTrue("(", getParen()); > This example breaks the current patch doesn't it? Again, I'm ok with this patch if it works and we can revisit later if needed. > -SCR > > On 2011/07/19 16:53:03, David Tseng wrote: > > True, but are we not just looking for strings of the pattern > > foo(...); > > i.e. > > functionName + "(" + "\s\S" + ");" > > > > Also, the article correctly prints out that this is hard to do with a true > > language semantic parser because of cases like: > > foo("I'm a string with ( a paren!"); > > On 2011/07/19 01:19:25, Sheridan Rawlins wrote: > > > Hah, that's what you'd think. Unfortunately, it's not true - read the > > following > > > link for more info. > > > > > > > > > http://www.texttoolkit.com/index.php?option=com_content&view=article&id=63:pa... > > > > > > On 2011/07/18 18:31:59, David Tseng wrote: > > > > nit: up to you, but it seems like we can do this much much more easily > with > > > > String.match and a regex. > > > > > >
I didn't really review the JS code, just one comment. http://codereview.chromium.org/7250009/diff/14001/chrome/test/data/webui/asse... File chrome/test/data/webui/assertions.js (right): http://codereview.chromium.org/7250009/diff/14001/chrome/test/data/webui/asse... chrome/test/data/webui/assertions.js:13: GEN('#include "chrome/browser/ui/webui/chrome_web_ui.h"'); I don't really like how C++ and JS code is mixed in this file, making it harder for syntax highlighters, indentation, and readability.
http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/5001/chrome/test/data/webui/test_... chrome/test/data/webui/test_api.js:247: 'matchedParamsIndex': function(string, index) { Mea Culpa; good catch, David. Yeah, short of being able to get the AST (I don't think that API is available within Javascript), any parsing is going to have gotchas. I like the regex of ending on ); but even that could have assertEquals(");", foo); Matching without being a parser is hard, and as long as we have to pick the lesser of two evils, maybe regex is easier to read/maintain. I just uploaded the RegExp version - it does seem much easier to understand. Done. On 2011/07/19 17:46:50, David Tseng wrote: > On 2011/07/19 16:57:40, Sheridan Rawlins wrote: > > What about > > assertTrue(expected, function(foo, bar))? > > You can make reg ex's lazy by using "?". Also note that we're matching on ");" > and not just ")" for any reg ex's. > > > > > > > I also wouldn't want to come back and debug if someone did something like you > > mentioned: > > assertTrue("(", getParen()); > > > > This example breaks the current patch doesn't it? > > Again, I'm ok with this patch if it works and we can revisit later if needed. > > -SCR > > > > On 2011/07/19 16:53:03, David Tseng wrote: > > > True, but are we not just looking for strings of the pattern > > > foo(...); > > > i.e. > > > functionName + "(" + "\s\S" + ");" > > > > > > Also, the article correctly prints out that this is hard to do with a true > > > language semantic parser because of cases like: > > > foo("I'm a string with ( a paren!"); > > > On 2011/07/19 01:19:25, Sheridan Rawlins wrote: > > > > Hah, that's what you'd think. Unfortunately, it's not true - read the > > > following > > > > link for more info. > > > > > > > > > > > > > > http://www.texttoolkit.com/index.php?option=com_content&view=article&id=63:pa... > > > > > > > > On 2011/07/18 18:31:59, David Tseng wrote: > > > > > nit: up to you, but it seems like we can do this much much more easily > > with > > > > > String.match and a regex. > > > > > > > > > > http://codereview.chromium.org/7250009/diff/14001/chrome/test/data/webui/asse... File chrome/test/data/webui/assertions.js (right): http://codereview.chromium.org/7250009/diff/14001/chrome/test/data/webui/asse... chrome/test/data/webui/assertions.js:13: GEN('#include "chrome/browser/ui/webui/chrome_web_ui.h"'); In getting the generator to work, we didn't want to have to include a generated header from C++, but maybe the javascript could include an -inl.h file which has the required C++-context code. That seems reasonable, I'll upload that change; tell me what you think. Done. On 2011/07/20 16:58:49, Paweł Hajdan Jr. wrote: > I don't really like how C++ and JS code is mixed in this file, making it harder > for syntax highlighters, indentation, and readability.
While working on another print_preview test, I realized that you need the actual value of the message in the Error to support constructed messages. Please re-LG this as it has changed both in using RegExp, and in printing the message. Thanks.
LGTM http://codereview.chromium.org/7250009/diff/27001/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7250009/diff/27001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:225: /** nit: new line before.
Try job failure for 7250009-27001 (retry) on linux_clang for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Try job failure for 7250009-31002 (retry) on linux for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
Change committed as 93769 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
