Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624913004:
Test that subpackages of front_end don't have undesired dependencies. (Closed)
|
Created:
3 years, 11 months ago by Paul Berry Modified:
3 years, 11 months ago CC:
reviews_dartlang.org, ahe Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionTest that subpackages of front_end don't have undesired dependencies.
R=danrubel@google.com
Committed: https://github.com/dart-lang/sdk/commit/9e4945164380878d4bd761901323e4d540d4c47e
Patch Set 1 #
Total comments: 12
Messages
Total messages: 9 (2 generated)
paulberry@google.com changed reviewers: + danrubel@google.com, sigmund@google.com
https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... File pkg/front_end/test/subpackage_relationships_test.dart (right): https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:27: final subpackageRules = { Note: this describes the dependencies that exist today. My expectation is that we'll update this variable as necessary whenever new subpackages are added or the dependencies change.
LGTM with comment for you to address as you see fit. https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... File pkg/front_end/test/subpackage_relationships_test.dart (right): https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:54: final frontEndRootUri = Platform.script.resolve('..'); Will this test fail if run from the wrong directory? Should we add a fast fail sanity check?
https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... File pkg/front_end/test/subpackage_relationships_test.dart (right): https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:54: final frontEndRootUri = Platform.script.resolve('..'); On 2017/01/12 21:54:54, danrubel wrote: > Will this test fail if run from the wrong directory? > Should we add a fast fail sanity check? Platform.script is the path to the script file (meaning this file), so it should be fine regardless of which directory the script is run from.
Description was changed from ========== Test that subpackages of front_end don't have undesired dependencies. ========== to ========== Test that subpackages of front_end don't have undesired dependencies. R=danrubel@google.com Committed: https://github.com/dart-lang/sdk/commit/9e4945164380878d4bd761901323e4d540d4c47e ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 9e4945164380878d4bd761901323e4d540d4c47e (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... File pkg/front_end/test/subpackage_relationships_test.dart (right): https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:91: Future<List<Uri>> findFrontEndUris() async { FWIW - for simple unit tests, I'm in favor of keeping things simple and use sync IO if that helps. https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:99: .replaceAll(pathos.separator, '/'); alternatively, use the various pathos contexts (in this case pathos.url)? pathos.url.joinAll(pathos.split(pathos.relative(...))); https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:100: if (!posixRelativePath.startsWith('lib/')) continue; consider making the frontEndRootPath be `Platform.script.resolve('../lib/');`, then you can focus the crawling to just the lib folder. https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:124: for (var i = 0; i < graph.topologicallySortedCycles.length; i++) { Do we need to use the graph/walker API and compute SSCs? If it is sufficient, it would be nice to only iterate over the files on pkg/front_end and check URIs by parsing directives directly, without chasing dependencies into other packages entirely.
Message was sent while issue was closed.
Comments marked "Done" are addressed in https://codereview.chromium.org/2627143004 https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... File pkg/front_end/test/subpackage_relationships_test.dart (right): https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:91: Future<List<Uri>> findFrontEndUris() async { On 2017/01/12 22:39:12, Siggi Cherem (dart-lang) wrote: > FWIW - for simple unit tests, I'm in favor of keeping things simple and use sync > IO if that helps. Fair enough. Done. https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:99: .replaceAll(pathos.separator, '/'); On 2017/01/12 22:39:12, Siggi Cherem (dart-lang) wrote: > alternatively, use the various pathos contexts (in this case pathos.url)? > > pathos.url.joinAll(pathos.split(pathos.relative(...))); Done. https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:100: if (!posixRelativePath.startsWith('lib/')) continue; On 2017/01/12 22:39:12, Siggi Cherem (dart-lang) wrote: > consider making the frontEndRootPath be `Platform.script.resolve('../lib/');`, > then you can focus the crawling to just the lib folder. Done. https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:124: for (var i = 0; i < graph.topologicallySortedCycles.length; i++) { On 2017/01/12 22:39:12, Siggi Cherem (dart-lang) wrote: > Do we need to use the graph/walker API and compute SSCs? > > If it is sufficient, it would be nice to only iterate over the files on > pkg/front_end and check URIs by parsing directives directly, without chasing > dependencies into other packages entirely. Regarding computation of library cycles, I agree that it's not needed but I think there's little harm in using an API that computes it as a side effect (the algorithm used by the front end is O(N)). I'd like to continue using the graph/walker API on the grounds that we should eat our own dogfood as much as possible. Regarding the fact that we waste our time iterating over files in other packages, I agree that this is unfortunate. I plan to address this in a future CL by implementing support for CompilerOptions.chaseDependencies = false in the graph/walker API, and that will prevent it from looking into any files other than the ones in front_end/lib.
Message was sent while issue was closed.
thanks! https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... File pkg/front_end/test/subpackage_relationships_test.dart (right): https://codereview.chromium.org/2624913004/diff/1/pkg/front_end/test/subpacka... pkg/front_end/test/subpackage_relationships_test.dart:124: for (var i = 0; i < graph.topologicallySortedCycles.length; i++) { On 2017/01/12 23:35:57, Paul Berry wrote: > On 2017/01/12 22:39:12, Siggi Cherem (dart-lang) wrote: > > Do we need to use the graph/walker API and compute SSCs? > > > > If it is sufficient, it would be nice to only iterate over the files on > > pkg/front_end and check URIs by parsing directives directly, without chasing > > dependencies into other packages entirely. > > Regarding computation of library cycles, I agree that it's not needed but I > think there's little harm in using an API that computes it as a side effect (the > algorithm used by the front end is O(N)). I'd like to continue using the > graph/walker API on the grounds that we should eat our own dogfood as much as > possible. Sounds good, no problem. Last year some unit tests were blocking some work in dart2js because of extra dependencies that weren't necessary, so I'm now more alert when it comes to unit tests dependencies :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
