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

Issue 9147058: Hook up the dartdoc tests to test.py, test.dart, and frog/presubmit.py. (Closed)

Created:
8 years, 11 months ago by Bob Nystrom
Modified:
8 years, 11 months ago
CC:
reviews_dartlang.org, Mads Ager (google), Siggi Cherem (dart-lang)
Visibility:
Public.

Description

Hook up the dartdoc tests to test.py, test.dart, and frog/presubmit.py. Committed: https://code.google.com/p/dart/source/detail?r=3207

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fix test names. #

Total comments: 4

Patch Set 3 : Respond to review. #

Patch Set 4 : Rebase and upload. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+97 lines, -1018 lines) Patch
M frog/presubmit.py View 1 2 1 chunk +1 line, -1 line 0 comments Download
M tools/test.dart View 2 chunks +4 lines, -0 lines 0 comments Download
M tools/testing/dart/test_options.dart View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M utils/dartdoc/html_renderer.dart View 1 2 1 chunk +6 lines, -1 line 0 comments Download
D utils/dartdoc/test/dartdoc_tests.dart View 1 chunk +0 lines, -176 lines 0 comments Download
D utils/dartdoc/test/dummy.dart View 1 chunk +0 lines, -16 lines 0 comments Download
D utils/dartdoc/test/markdown_tests.dart View 1 chunk +0 lines, -806 lines 0 comments Download
A utils/tests/dartdoc/dartdoc.status View 1 2 1 chunk +23 lines, -0 lines 0 comments Download
A + utils/tests/dartdoc/src/dartdoc_tests.dart View 1 2 3 chunks +36 lines, -16 lines 0 comments Download
A + utils/tests/dartdoc/src/dummy.dart View 0 chunks +-1 lines, --1 lines 0 comments Download
A + utils/tests/dartdoc/src/markdown_tests.dart View 1 1 chunk +2 lines, -2 lines 0 comments Download
A utils/tests/dartdoc/test_config.dart View 1 chunk +17 lines, -0 lines 0 comments Download
A utils/tests/dartdoc/testcfg.py View 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
Bob Nystrom
8 years, 11 months ago (2012-01-11 01:59:15 UTC) #1
Siggi Cherem (dart-lang)
Looks good, but I'd wait for Bill's comments to make sure I didn't miss anything ...
8 years, 11 months ago (2012-01-11 18:49:13 UTC) #2
Jennifer Messerly
http://codereview.chromium.org/9147058/diff/1/utils/dartdoc/html_renderer.dart File utils/dartdoc/html_renderer.dart (right): http://codereview.chromium.org/9147058/diff/1/utils/dartdoc/html_renderer.dart#newcode38 utils/dartdoc/html_renderer.dart:38: final attributeNames = new List.from(element.attributes.getKeys()); Frog has an "orderValuesByKeys" ...
8 years, 11 months ago (2012-01-11 19:15:10 UTC) #3
Bill Hesse
LGTM. http://codereview.chromium.org/9147058/diff/6001/utils/dartdoc/test/dartdoc_tests.dart File utils/dartdoc/test/dartdoc_tests.dart (left): http://codereview.chromium.org/9147058/diff/6001/utils/dartdoc/test/dartdoc_tests.dart#oldcode11 utils/dartdoc/test/dartdoc_tests.dart:11: // TODO(rnystrom): Better path to unittest. Let me ...
8 years, 11 months ago (2012-01-11 21:01:13 UTC) #4
Bob Nystrom
8 years, 11 months ago (2012-01-11 21:28:47 UTC) #5
Thanks!

http://codereview.chromium.org/9147058/diff/1/utils/dartdoc/html_renderer.dart
File utils/dartdoc/html_renderer.dart (right):

http://codereview.chromium.org/9147058/diff/1/utils/dartdoc/html_renderer.dar...
utils/dartdoc/html_renderer.dart:38: final attributeNames = new
List.from(element.attributes.getKeys());
On 2012/01/11 19:15:10, John Messerly wrote:
> Frog has an "orderValuesByKeys" method we use when we want just the values
> (frog/utils.dart). You could add a version called something like "orderKeys":
> 
> /** Sorts the map by the key. */
> List orderKeys(Map map) {
>   // TODO(jmesserly): it'd be nice to have SortedMap in corelib.
>   List keys = map.getKeys();
>   keys.sort((x, y) => x.compareTo(y));
>   return keys;
> }

Dartdoc is coupled to frog but this little markdown sublibrary isn't. I could
pull this out into its own function but that seemed like overkill for now.

> 
> then use it like:
> 
> for (name in orderKeys(element.attributes)) { ... }
> 
> btw, you shouldn't need to copy the result of getKeys. For better or worse, it
> already does that in the current design (and if it changes to return an O(1)
> wrapper, we'll have to fix other places in the code, so IMO it's safe to rely
on

Changed, but man that feels gross. :( It should return an O(1) immutable
collection.

> that fact).

http://codereview.chromium.org/9147058/diff/1/utils/tests/dartdoc/dartdoc.status
File utils/tests/dartdoc/dartdoc.status (right):

http://codereview.chromium.org/9147058/diff/1/utils/tests/dartdoc/dartdoc.sta...
utils/tests/dartdoc/dartdoc.status:13: [ $component == frogium || $component ==
dartium ]
On 2012/01/11 18:49:14, sigmund wrote:
> could you please include here $component == webdriver too? We plan to enable
> that later today.

Done.

http://codereview.chromium.org/9147058/diff/1/utils/tests/dartdoc/src/dartdoc...
File utils/tests/dartdoc/src/dartdoc_tests.dart (right):

http://codereview.chromium.org/9147058/diff/1/utils/tests/dartdoc/src/dartdoc...
utils/tests/dartdoc/src/dartdoc_tests.dart:120: // TODO(rnystrom): Disabling
these for now. The problem is that loading
On 2012/01/11 19:15:10, John Messerly wrote:
> Up to you, but one thought I had here: instead of commenting it out, you could
> change the test to catch the error and print a diagnostic that it was "run
from
> the wrong path". Ensures the code still compiles & will run correctly if it is
> started from the right path. (I've been admonished in the past by test folks
> that disabling tests via commenting out is bad, because the code bit rots, and
> then never get resurrected because it's too broken.)
> 
> The "console" and "process" node.js tests have an interesting "solution" ...
> they look for the file in a few different relative paths.
> 
> also we should have a bug filed about the lack of a way to get the script's
path
> on the VM.

Done: https://code.google.com/p/dart/issues/detail?id=1145

http://codereview.chromium.org/9147058/diff/6001/utils/dartdoc/test/dartdoc_t...
File utils/dartdoc/test/dartdoc_tests.dart (left):

http://codereview.chromium.org/9147058/diff/6001/utils/dartdoc/test/dartdoc_t...
utils/dartdoc/test/dartdoc_tests.dart:11: // TODO(rnystrom): Better path to
unittest.
On 2012/01/11 21:01:13, Bill Hesse wrote:
> Let me know when you find a better way to do paths.  I haven't found any.

I don't think we'll be able to do better than this without a language change.

http://codereview.chromium.org/9147058/diff/6001/utils/tests/dartdoc/src/dart...
File utils/tests/dartdoc/src/dartdoc_tests.dart (right):

http://codereview.chromium.org/9147058/diff/6001/utils/tests/dartdoc/src/dart...
utils/tests/dartdoc/src/dartdoc_tests.dart:121: // dummy.dart is sensitive to
the location that dart was invoked from and
On 2012/01/11 21:01:13, Bill Hesse wrote:
> Can we add a bug reference here, to a bug that says that we will get the
client
> directory and the current directory in test.py, when we implement getting the
> script path in the script arguments, and to fix it then.  I'll file the bug
now.

Done and filed already: https://code.google.com/p/dart/issues/detail?id=1145

:)

Powered by Google App Engine
This is Rietveld 408576698