Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1993643002:
Add test of some package-config configurations. (Closed)
|
Created:
4 years, 7 months ago by Lasse Reichstein Nielsen Modified:
4 years, 6 months ago Reviewers:
floitsch CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd test of some package-config configurations.
More to come.
R=floitsch@google.com
Committed: https://github.com/dart-lang/sdk/commit/39df5d56b27d4971297e2cb51dc097f868b79dde
Patch Set 1 #Patch Set 2 : Fix type #
Total comments: 15
Patch Set 3 : Address comments #
Total comments: 11
Patch Set 4 : Remove legacy return statement from void function. #Messages
Total messages: 10 (2 generated)
lrn@google.com changed reviewers: + floitsch@google.com
LGTM, although the ping-pong makes it really hard to understand. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... File tests/standalone/packages_file_test.dart (right): https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:16: for (var test in [testDirect]) { As long as there is just one, I would just rename 'testDirect' to `test` and remove the `for` loop. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:19: await run(dir.resolve("main.dart"), {"foo.x": null}); This ping-pong is too complicated. Just add a token that says "resolve wrt dir". You could also say that you always run the first file. If it's in the map, then you concatenate (as expected). For example: test("file: no resolution", {"main": testMain}, {"foo.x": null}); test("http: no resolution", {"main": testMain}, { "foo/": "#-packages/foo/", "foo/bar": "#-packages/foo/bar", "foo.x": null}); https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:83: await test("$scheme: explicit package root, no slash", (run, dir) async { long line. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:187: // Helper fnuctionality. functionality https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:196: runTest(main, expectations, {Uri root, Uri config}) async { Please keep one line before and after nested functions. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:226: await test(runTest, tmpDir.uri, serverUri(https)); You don't seem to use this case. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:229: print("ERROR in $name: $e"); I would rethrow. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:287: "foo": res1?.toString(), No need for "?.". Null supports 'toString' too. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:364: Directory createDir(Directory base, String name) { New lines before and after nested functions. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:388: var tempDir = Directory.systemTemp.createTempSync("pftest-${tmpDirCounter++}-"); long line.
https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... File tests/standalone/packages_file_test.dart (right): https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:19: await run(dir.resolve("main.dart"), {"foo.x": null}); On 2016/05/18 13:36:32, floitsch wrote: > This ping-pong is too complicated. And I haven't even gotten started on nested calls yet (running the test in a spawned isolate). That's probably going to be really complicated :) > > Just add a token that says "resolve wrt dir". > You could also say that you always run the first file. If it's in the map, then > you concatenate (as expected). I prefer not to rely on the ordering of maps, especially when you have nested maps. > > For example: > > test("file: no resolution", {"main": testMain}, {"foo.x": null}); > > test("http: no resolution", {"main": testMain}, { > "foo/": "#-packages/foo/", > "foo/bar": "#-packages/foo/bar", > "foo.x": null}); Or probably something like "%{....}" so it has a clear delimiter, since I'll also need to replace inside larger texts (at least the .packages files). It could probably work. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:226: await test(runTest, tmpDir.uri, serverUri(https)); Yet :) I need to have cases where the script is file: and the .packages or packages/ is http: and vice versa. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:229: print("ERROR in $name: $e"); Whoops, left over debug print. https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:287: "foo": res1?.toString(), But that would make at the string "null", not the value null. It might not make a difference if I don't generate the string "null" anywhere else, but I prefer to keep it separate.
https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... File tests/standalone/packages_file_test.dart (right): https://codereview.chromium.org/1993643002/diff/20001/tests/standalone/packag... tests/standalone/packages_file_test.dart:287: "foo": res1?.toString(), On 2016/05/18 14:01:16, Lasse Reichstein Nielsen wrote: > But that would make at the string "null", not the value null. > It might not make a difference if I don't generate the string "null" anywhere > else, but I prefer to keep it separate. right.
PTAL
LGTM. much better. thanks. https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... File tests/standalone/packages_file_test.dart (right): https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:16: await test("file: no resolution", Add a comment on the first test that explains that '%file' and '%http' are replaced with the file and http root (respectively). https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:55: ".packages": ""}; Provide comment what an empty ".packages" should do. (It seems like we just ignore it then). https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:168: // With a non-file: URI, the lookup assumes a packges/ dir. packages https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:203: String httpRoot = "<not http server configured>"; no https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:324: const String spawnUriMain = r""" unused. https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:342: const String spawnMain = r""" unused.
Description was changed from ========== Add test of some package-config configurations. More to come. ========== to ========== Add test of some package-config configurations. More to come. R=floitsch@google.com Committed: https://github.com/dart-lang/sdk/commit/39df5d56b27d4971297e2cb51dc097f868b79dde ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 39df5d56b27d4971297e2cb51dc097f868b79dde (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... File tests/standalone/packages_file_test.dart (right): https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:55: ".packages": ""}; Will do. We don't ignore it - the test should not find and use it, but if it did, it would mean that no package names could be resolved. https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:168: // With a non-file: URI, the lookup assumes a packges/ dir. I like to use .packages to signify the file and packages/ to signify the "packages" directory. https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:203: String httpRoot = "<not http server configured>"; Will do. https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:324: const String spawnUriMain = r""" Yes. Reserved for later use. That's the part I'm going to work on immediately after this. https://codereview.chromium.org/1993643002/diff/40001/tests/standalone/packag... tests/standalone/packages_file_test.dart:342: const String spawnMain = r""" Ditto. |
