Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8394043:
Start adding files for Dart version of test script test.py. (Closed)
|
Created:
9 years, 1 month ago by William Hesse Modified:
9 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionStart adding files for Dart version of test script test.py.
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=885
Patch Set 1 #
Total comments: 38
Patch Set 2 : Addressed comments, added and tested boolean variables. #
Total comments: 19
Patch Set 3 : Use CamelCase, test evaluation, move tests to right directory. #Patch Set 4 : Rename file #
Messages
Total messages: 10 (0 generated)
I may remove the toString methods on the expression AST classes. To make toString print out the formula in the original source format, complicated tracking of precedence (to add parentheses) is needed. Right now, they print out in constructor format.
DBC http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... File tools/testing/dart/status_expressions_lib.dart (right): http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:8: * Parse and evaluate expressions in a .status file for Dart and V8. Extra space. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... File tools/testing/dart/test_status_expressions.dart (right): http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... tools/testing/dart/test_status_expressions.dart:30: print(ast.toString()); You should not need to call toString when giving something to print.
http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... File tools/testing/dart/status_expressions_lib.dart (right): http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:21: * Production are listed in order of precedence, and the || and , operators Remove the extra leading space starting from here as well? http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:62: // A set of test outcomes. Please update the comment here to actually describe the method. Or delete the comment. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:74: String toString() => "==(\$${left.name}, ${right.value})"; Why not just "(\$${left.name} == ${right.value})"? http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:102: String toString() => "$op(${left.toString()},${right.toString()})"; I would go for infix here and in the rest. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:139: /* An iterator that provides peeking at the current token. */ I think you should either make this a doc comment and use /** or just use //. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:146: //print(tokens[0] + tokens[1]); remove. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:148: current = token_iterator.next(); Shouldn't this just call advance? http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:149: //print(current); remove. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:156: print(current); remove http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:202: "Missing right parenthesis in expression"); Please align with first parameter. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:207: "Expected identifier in expression, got " + scanner.current); Please align with first parameter or move both to new lines. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:241: "Missing right parenthesis in expression"); align http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:248: @"Expected $identifier in expression, got " + scanner.current); $identifier -> $ http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... File tools/testing/dart/test_status_expressions.dart (right): http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... tools/testing/dart/test_status_expressions.dart:1: // Copyright (c) 2011, the Dart project authors. Please see the AUTHORS file We need to create a new class of tests in dart/test/testing or something like that so that these tests are run when using the current python script. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... tools/testing/dart/test_status_expressions.dart:30: print(ast.toString()); Just remove the print? However, you should verify that the AST that you get here is as you expect using Expect calls. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... tools/testing/dart/test_status_expressions.dart:39: ")", "&&", "\$", "mode", "==", "release"]); Parse and verify as well? http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... tools/testing/dart/test_status_expressions.dart:42: static void test_3() { Use Expect calls so this test can be run by the test scripts and fail. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... tools/testing/dart/test_status_expressions.dart:52: static void test_4() { Ditto. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/test_status_... tools/testing/dart/test_status_expressions.dart:70: print(ast.toString()); Verify the AST with Expect calls.
http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... File tools/testing/dart/status_expressions_lib.dart (right): http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:37: Tokenizer(String this.expression) Fit on one line? http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:41: * Tokens are : "&&", "||", "==", "$", "(", ")", ",", and (maximal) \w+. Maybe list the tokens in the same order as they are matched by the regexp. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:140: class Scanner { Should we have token constants? class Token { final String AND = "&&"; final String OR = "||"; ... } http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:161: Scanner scanner; Empty line before the constructor. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:258: @"Expected identifier in expression, got " + scanner.current); No need for @ here.
Addressed all comments, making all suggested changes. Added standalone boolean variables, such as in $arch == ia32 || $checked, to parser. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... File tools/testing/dart/status_expressions_lib.dart (right): http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:139: /* An iterator that provides peeking at the current token. */ On 2011/10/26 12:37:47, Mads Ager wrote: > I think you should either make this a doc comment and use /** or just use //. Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:140: class Scanner { On 2011/10/26 13:36:24, Søren Gjesse wrote: > Should we have token constants? > > class Token { > final String AND = "&&"; > final String OR = "||"; > ... > > } Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:146: //print(tokens[0] + tokens[1]); On 2011/10/26 12:37:47, Mads Ager wrote: > remove. Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:148: current = token_iterator.next(); On 2011/10/26 12:37:47, Mads Ager wrote: > Shouldn't this just call advance? Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:149: //print(current); On 2011/10/26 12:37:47, Mads Ager wrote: > remove. Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:156: print(current); On 2011/10/26 12:37:47, Mads Ager wrote: > remove Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:161: Scanner scanner; On 2011/10/26 13:36:24, Søren Gjesse wrote: > Empty line before the constructor. Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:202: "Missing right parenthesis in expression"); On 2011/10/26 12:37:47, Mads Ager wrote: > Please align with first parameter. Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:207: "Expected identifier in expression, got " + scanner.current); On 2011/10/26 12:37:47, Mads Ager wrote: > Please align with first parameter or move both to new lines. Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:241: "Missing right parenthesis in expression"); On 2011/10/26 12:37:47, Mads Ager wrote: > align Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:248: @"Expected $identifier in expression, got " + scanner.current); On 2011/10/26 12:37:47, Mads Ager wrote: > $identifier -> $ Done. http://codereview.chromium.org/8394043/diff/1/tools/testing/dart/status_expre... tools/testing/dart/status_expressions_lib.dart:258: @"Expected identifier in expression, got " + scanner.current); On 2011/10/26 13:36:24, Søren Gjesse wrote: > No need for @ here. Done.
We are getting there. The code is looking good. We need to get the test run automatically and test the evaluate method. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... File tools/testing/dart/status_expressions_lib.dart (right): http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:79: Two blank lines? http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:87: Two blank lines? http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:88: class TermConstant { Should this just be named Constant or should Variable be TermVariable? http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... File tools/testing/dart/test_status_expressions.dart (right): http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:1: // Copyright (c) 2011, the Dart project authors. Please see the AUTHORS file This file should be moved to tests/standalone/TestStatusExpressionsTest.dart so it is run automatically by the test scripts. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:7: main() { I would move main to the bottom of the file. I believe that is the style used in the rest of the tests. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:33: ast.toString()); For all of the ASTs that we generate we should call evaluate in a couple of environments to test that evaluate works as expected. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:48: @" $mode == debug && ($arch == chromium || *$arch == dartc) "); Extract this string into a variable and use it in the Expect equals below? http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:63: @"($arch == (-dartc || $arch == chromium) && $mode == release"); Same here.
http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... File tools/testing/dart/status_expressions_lib.dart (right): http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:211: if (scanner.current == "(") { Token for (. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:214: Expect.equals(scanner.current, ")", and ). http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:268: if (scanner.current == "==") { Token for ==
Drive-by nits: http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... File tools/testing/dart/status_expressions_lib.dart (right): http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:11: * BooleanExpression := $variable_name == value | camelCase http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/status_ex... tools/testing/dart/status_expressions_lib.dart:157: Iterator token_iterator; camelCase http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... File tools/testing/dart/test_status_expressions.dart (right): http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:13: test_1(); camelCase -- no underscores
Tests moved to test directory, evaluation of expressions tested, and CamelCase used everywhere. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... File tools/testing/dart/test_status_expressions.dart (right): http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:1: // Copyright (c) 2011, the Dart project authors. Please see the AUTHORS file On 2011/10/27 07:07:47, Mads Ager wrote: > This file should be moved to tests/standalone/TestStatusExpressionsTest.dart so > it is run automatically by the test scripts. Done. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:7: main() { On 2011/10/27 07:07:47, Mads Ager wrote: > I would move main to the bottom of the file. I believe that is the style used in > the rest of the tests. Done. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:33: ast.toString()); On 2011/10/27 07:07:47, Mads Ager wrote: > For all of the ASTs that we generate we should call evaluate in a couple of > environments to test that evaluate works as expected. Done. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:48: @" $mode == debug && ($arch == chromium || *$arch == dartc) "); On 2011/10/27 07:07:47, Mads Ager wrote: > Extract this string into a variable and use it in the Expect equals below? Done. http://codereview.chromium.org/8394043/diff/6001/tools/testing/dart/test_stat... tools/testing/dart/test_status_expressions.dart:63: @"($arch == (-dartc || $arch == chromium) && $mode == release"); On 2011/10/27 07:07:47, Mads Ager wrote: > Same here. Done.
LGTM when final comment addressed. You should change the library implementation file back to using lower-case and underscores. We really should be using that for the file names in the test directory as well. The file name used to be used to find the name of the class on which to invoke a method. This is no longer the case and all the test files should change names to use lower-case and underscores. But that should be done in a separate change. Sorry about that, I wasn't clear in my last round of comments. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
