Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(254)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2915843003: Simplify test_suite.dart. (Closed)

Created:
3 years, 6 months ago by Bob Nystrom
Modified:
3 years, 6 months ago
Reviewers:
Bill Hesse
CC:
reviews_dartlang.org
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Simplify test_suite.dart. - Get rid of baseCommand, which is only ever an empty list. - Move some utility classes out of the giant test_suite.dart. R=whesse@google.com Committed: https://github.com/dart-lang/sdk/commit/19cb0cafb712b0be9b6a0a74d8d974bbc69b3010

Patch Set 1 #

Total comments: 4

Patch Set 2 : Re-add some type annotations. #

Patch Set 3 : Fix path in test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+311 lines, -317 lines) Patch
M tests/standalone/io/skipping_dart2js_compilations_test.dart View 4 chunks +8 lines, -12 lines 0 comments Download
M tests/standalone/io/test_runner_test.dart View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M tools/testing/dart/co19_test.dart View 1 chunk +1 line, -1 line 0 comments Download
M tools/testing/dart/compiler_configuration.dart View 2 chunks +2 lines, -1 line 0 comments Download
M tools/testing/dart/configuration.dart View 1 1 chunk +1 line, -1 line 0 comments Download
M tools/testing/dart/drt_updater.dart View 1 chunk +1 line, -1 line 0 comments Download
M tools/testing/dart/http_server.dart View 1 chunk +0 lines, -1 line 0 comments Download
M tools/testing/dart/main.dart View 1 chunk +1 line, -1 line 0 comments Download
M tools/testing/dart/options.dart View 1 1 chunk +1 line, -1 line 0 comments Download
M tools/testing/dart/package_testing_support.dart View 1 chunk +1 line, -1 line 0 comments Download
M tools/testing/dart/runtime_configuration.dart View 1 chunk +1 line, -0 lines 0 comments Download
M tools/testing/dart/test_progress.dart View 1 chunk +0 lines, -1 line 0 comments Download
M tools/testing/dart/test_suite.dart View 1 13 chunks +21 lines, -296 lines 0 comments Download
M tools/testing/dart/utils.dart View 2 chunks +272 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (2 generated)
Bob Nystrom
3 years, 6 months ago (2017-05-31 22:47:59 UTC) #2
Bob Nystrom
Friendly ping!
3 years, 6 months ago (2017-06-08 23:41:01 UTC) #3
Bill Hesse
lgtm
3 years, 6 months ago (2017-06-09 09:05:20 UTC) #4
Bill Hesse
Comments added to the LGTM: https://codereview.chromium.org/2915843003/diff/1/tools/testing/dart/test_suite.dart File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/2915843003/diff/1/tools/testing/dart/test_suite.dart#newcode759 tools/testing/dart/test_suite.dart:759: var segments = filePath.directoryPath.relativeTo(root).segments(); ...
3 years, 6 months ago (2017-06-09 13:27:13 UTC) #5
Bob Nystrom
Committed patchset #3 (id:40001) manually as 19cb0cafb712b0be9b6a0a74d8d974bbc69b3010 (presubmit successful).
3 years, 6 months ago (2017-06-09 23:46:53 UTC) #7
Bob Nystrom
3 years, 6 months ago (2017-06-15 21:57:44 UTC) #8
Message was sent while issue was closed. 
https://codereview.chromium.org/2915843003/diff/1/tools/testing/dart/test_sui...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/2915843003/diff/1/tools/testing/dart/test_sui...
tools/testing/dart/test_suite.dart:759: var segments =
filePath.directoryPath.relativeTo(root).segments();
On 2017/06/09 13:27:13, Bill Hesse wrote:
> I don't agree with dropping the type here.

Added it back, though I personally prefer omitting it.

https://codereview.chromium.org/2915843003/diff/1/tools/testing/dart/test_sui...
tools/testing/dart/test_suite.dart:776: var expectations =
testExpectations.expectations(testName);
On 2017/06/09 13:27:13, Bill Hesse wrote:
> I don't agree with dropping the type here.  I know the style guide (that you
> wrote) says to do it, but it just makes the code harder for me to read and
> understand.

Re-added.

Powered by Google App Engine
This is Rietveld 408576698