Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8715006:
tools/test.dart: Add multitest support to Dart implementation of test runner. (Closed)
|
Created:
9 years ago by Bill Hesse Modified:
9 years ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptiontools/test.dart: Add multitest support to Dart implementation of test runner.
BUG=
TEST=tools/test.dart
Committed: https://code.google.com/p/dart/source/detail?r=1919
Patch Set 1 #
Total comments: 30
Patch Set 2 : All comments addressed. Tests run with 0 failures. #
Total comments: 18
Patch Set 3 : Address comments. #
Messages
Total messages: 6 (0 generated)
I welcome suggestions about the FileThen interface, or about Async( fooAsync ).thenHandler( handlerSetter); http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/test_suite.dart File tools/testing/dart/test_suite.dart (right): http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/test_suite.d... tools/testing/dart/test_suite.dart:233: if (vmOptionsList.isEmpty()) { This if statement can be removed now.
http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.dart File tools/testing/dart/multitest.dart (right): http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:7: //#import("status_file_parser.dart"); Code in comments. Remove? http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:31: if (lines.last() == '') lines.removeLast(); Do you need to do this? If you do, don't you need to do it in a loop to get rid of all the blank lines at the end of the file? http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:62: tests[''] = testTemplate; What is this used for. Can you add a comment about the test with the empty string as key? http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:69: print(''); Remove printing. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:81: Map tests = ExtractTestsFromMultitest(filename); You could pass in the maps to be filled out here as well. So you get a test map and an outcomes map. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:88: AddErrorHandler(dir, "Error creating temp directory"); I would just add the error handler. Abstracting that out makes it harder for me to understand. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:90: Async((){dir.createTemp();}).thenHandler((){ Urgh. I don't like this at all. I find it very hard to read and I don't see what it adds. Please just use the APIs directly. dir.createTempHandler = () { }; dir.createTemp(); http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:96: //RegisterFileForDeletion('$path$baseFilename$key.dart'); Code in comments. And below as well. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:103: FileThen file = new FileThen(new File(filename)); Why are you inventing new stuff? We have everything we need for async file operations already. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:104: AddErrorHandler(file, "Error creating temp file"); Just add the error handler? http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:105: file.createThen((){ Please use our APIs for this. Don't create extra layers here. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:106: file.openThen(file.WRITABLE, (){ file.openHandler = () { ... }; file.open(writable: true); http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:123: void AddErrorHandler(object, String error_string) { Please get rid of all of this stuff. There is no need for it. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/test_suite.dart File tools/testing/dart/test_suite.dart (right): http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/test_suite.d... tools/testing/dart/test_suite.dart:194: Function doTestBound(filename, isNegative, isNegativeIfChecked) { What does the 'Bound' part of this name mean? scheduleTest or something? http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/test_suite.d... tools/testing/dart/test_suite.dart:243: print("vmOptions added, filename = $filename"); Remove debug printing.
http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.dart File tools/testing/dart/multitest.dart (right): http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:7: //#import("status_file_parser.dart"); Remove these. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:10: A short comment here showing what is actually happening will be helpful, e.g. aaa xxx /// 1 yyy /// 2 bbb turns into: <outcome here> http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:69: print(''); Remove print. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:90: Async((){dir.createTemp();}).thenHandler((){ See comment below. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:96: //RegisterFileForDeletion('$path$baseFilename$key.dart'); Code in comments. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:98: //RegisterDirForDeletion(dir.path); Ditto. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:105: file.createThen((){ I think it would me much more readable to have a number of small functions that "tail calls" one after the other through the handler associated with the operation performed. File file ... void step1() { ... file.xxxHandler = step2; file.xxx } void step2() { ... file.yyyHandler = step3; file.yyy } void step3 { ... } Maybe tou need some kind of state object holding the File object together with other tracking stuff. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:180: // Alternatively, we could write Code in comments.
http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.dart File tools/testing/dart/multitest.dart (right): http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:7: //#import("status_file_parser.dart"); On 2011/11/29 08:35:30, Søren Gjesse wrote: > Remove these. Done. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:10: On 2011/11/29 08:35:30, Søren Gjesse wrote: > A short comment here showing what is actually happening will be helpful, e.g. > > aaa > xxx /// 1 > yyy /// 2 > bbb > > turns into: > > <outcome here> Done. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:31: if (lines.last() == '') lines.removeLast(); On 2011/11/29 08:12:04, Mads Ager wrote: > Do you need to do this? If you do, don't you need to do it in a loop to get rid > of all the blank lines at the end of the file? This should just get rid of a spurious empty line that is not in the original file, but is introduced by "split". If the file ends in \n, then the split will introduce an empty string after it. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:62: tests[''] = testTemplate; The key is now "none". There is always a "none" multitest created, with none of the multitest lines in it. On 2011/11/29 08:12:04, Mads Ager wrote: > What is this used for. Can you add a comment about the test with the empty > string as key? http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:69: print(''); On 2011/11/29 08:35:30, Søren Gjesse wrote: > Remove print. Done. http://codereview.chromium.org/8715006/diff/1/tools/testing/dart/multitest.da... tools/testing/dart/multitest.dart:81: Map tests = ExtractTestsFromMultitest(filename); On 2011/11/29 08:12:04, Mads Ager wrote: > You could pass in the maps to be filled out here as well. So you get a test map > and an outcomes map. Done. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... File tools/testing/dart/test_suite.dart (right): http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... tools/testing/dart/test_suite.dart:189: Function doTestBound(filename, isNegative, isNegativeIfChecked) { Name changed to createTestCase(). Not uploaded yet.
LGTM We can submit this in the current form. However, we should address these comments and the cleanup of temporary directories and files ASAP. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... File tools/testing/dart/multitest.dart (right): http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:46: Map<String, String> tests, Identation. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:85: Expect.equals('continued', rest); Should we always throw an exception here? Users will probably mostly run the test scripts without using checked mode. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:90: Expect.isTrue(validMultitestOutcomes.contains(rest)); Ditto. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:95: // TODO(whesse): Rewrite relative paths to reflect temporary directory. Why does this work if we don't do this? Where are the tests generated? We should make sure that they are generated in the output directory so we can clean everything generated from the repository by deleting the output dir. We can do that in the next change. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:128: Directory dir = new Directory(""); Could you add a TODO here. We should make sure to point this at the build output directory for test generation. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:130: (error) { Expect.fail("Error creating temp directory: $error"); }; Maybe just throw exception? http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:132: dir.createTempHandler = (){ space between '()' and '{' Also multiple occurrences below. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:170: outcome.contains('runtime error'); Indentation. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... File tools/testing/dart/test_suite.dart (right): http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... tools/testing/dart/test_suite.dart:189: Function doTestBound(filename, isNegative, isNegativeIfChecked) { We could use a named optional argument for isNegativeIfChecked. Alternatively, we could pass down enough information to determine if the test is negative in the multitest generation and avoid the extra parameter and logic here. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... tools/testing/dart/test_suite.dart:198: int middle = filename.lastIndexOf('_'); This will end up biting us because it is specific to the multi test case that you have just added. This will break matching for other simple test files that contain underscores. We need to find another way of mapping the multi tests to the expectations. At least we need to make sure that this only happens for the multi tests. Maybe a named optional argument that is the name used for test expectation matching. If nothing is supplied we use the filename otherwise we use the thing supplied? http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... tools/testing/dart/test_suite.dart:245: Expect.fail("empty vmOptionsList"); Remove this case completely and just replace with an Expect.isFalse or with always throwing an exception in this case?
http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... File tools/testing/dart/multitest.dart (right): http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:46: Map<String, String> tests, On 2011/11/30 09:11:23, Mads Ager wrote: > Identation. Done. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:85: Expect.equals('continued', rest); I was under the impression that Expect always checks the condition, in both production and checked mode. assert only applies in checked mode. I don't see anything in Expect's implementation that disables it, and we use it in all the tests. On 2011/11/30 09:11:23, Mads Ager wrote: > Should we always throw an exception here? Users will probably mostly run the > test scripts without using checked mode. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:95: // TODO(whesse): Rewrite relative paths to reflect temporary directory. Only one multitest has an #import, and so only that one fails, in the "none" default test, if we don't do this. We can leave it out, but then there is no place to put the comment. On 2011/11/30 09:11:23, Mads Ager wrote: > Why does this work if we don't do this? Where are the tests generated? We should > make sure that they are generated in the output directory so we can clean > everything generated from the repository by deleting the output dir. > > We can do that in the next change. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/multitest... tools/testing/dart/multitest.dart:128: Directory dir = new Directory(""); OK, I didn't think of using the build directory. Can we reliably find it? Do we really want to do this, and if we do, do we also want to delete the files and directory automatically, and do we want to try to ensure that we do it if the test is cancelled? What if someone wants to debug using a generated multitest? On 2011/11/30 09:11:23, Mads Ager wrote: > Could you add a TODO here. We should make sure to point this at the build output > directory for test generation. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... File tools/testing/dart/test_suite.dart (right): http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... tools/testing/dart/test_suite.dart:189: Function doTestBound(filename, isNegative, isNegativeIfChecked) { On 2011/11/30 09:11:23, Mads Ager wrote: > We could use a named optional argument for isNegativeIfChecked. Alternatively, > we could pass down enough information to determine if the test is negative in > the multitest generation and avoid the extra parameter and logic here. Done. http://codereview.chromium.org/8715006/diff/6002/tools/testing/dart/test_suit... tools/testing/dart/test_suite.dart:198: int middle = filename.lastIndexOf('_'); This only applies to multitests, because all other tests are in a src/ subdirectory. If this assumption is not true, then yes, we should do something different. But otherwise, I've just added a comment that only multitests get to the else clause. On 2011/11/30 09:11:23, Mads Ager wrote: > This will end up biting us because it is specific to the multi test case that > you have just added. This will break matching for other simple test files that > contain underscores. We need to find another way of mapping the multi tests to > the expectations. At least we need to make sure that this only happens for the > multi tests. Maybe a named optional argument that is the name used for test > expectation matching. If nothing is supplied we use the filename otherwise we > use the thing supplied? |
|||||||||||||||||||||||||||||||||||||||||||||||||||
