|
|
Created:
3 years, 5 months ago by bkonyi Modified:
3 years, 5 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdded functionality to dump relevant status file entries to a single file to make it easier to see which tests have which status file entries. Future work: remove entries from original status files.
BUG=
R=jcollins@google.com, rnystrom@google.com
Committed: https://github.com/dart-lang/sdk/commit/caf87da9ed218f45cd68ca2f52253c8b913eb53a
Patch Set 1 #
Total comments: 19
Patch Set 2 : Updated to use pkg:status_file, and updated to improve performance #
Total comments: 8
Patch Set 3 : Addressed additional comments #
Messages
Total messages: 11 (2 generated)
bkonyi@google.com changed reviewers: + jcollins@google.com, rnystrom@google.com
I've been finding digging around status files looking for particular tests really annoying, so I've updated the migration script to gather all the relevant entries for each test and then write them to a file for later reference. I plan on hopefully making further additions to cleanup the original status files of tests that have been migrated. Let me know if this is worth doing!
LGTM in general once these are fixed, this could definitely save us some time. If you're interested in removing old entries, perhaps one enhancement or separate tool you could make is to simply remove all entries that no longer match any files in the relevant directory. That would be useful for cleanup generally, possibly even beyond the life of our grouplet. https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... File tools/migration/lib/src/path.dart (right): https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... tools/migration/lib/src/path.dart:5: library legacy_path; Did this module get copied from somewhere else? https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... tools/migration/lib/src/path.dart:146: Path join(Path further) { Ordinarily, methods named "join" take an Iterable as a parameter, so calling this method join makes uses of the class harder to read. Can we either make this take an iterable or call it something else? Maybe joinWith? https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... tools/migration/lib/src/path.dart:154: if (hasTrailingSeparator) { This looks very much like append() from here out https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/sta... File tools/migration/lib/src/status_file.dart (right): https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/sta... tools/migration/lib/src/status_file.dart:9: /// Matches the header that begins a new section, like: There should probably be a TODO here to merge this code somehow with whatever code reads these files "for real".
I like the idea of having a separate tool to cleanup these status files, so I'll see if I can pull something together! Also, as I've mentioned in my comments, path.dart wasn't written by me (I just grabbed it from tools/testing/dart/path.dart), so I'm not really familiar with the code there myself. https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... File tools/migration/lib/src/path.dart (right): https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... tools/migration/lib/src/path.dart:5: library legacy_path; On 2017/07/24 18:24:51, jcollins wrote: > Did this module get copied from somewhere else? Yes, this is from the test runner code. I grabbed the status_file.dart from there and removed a lot of the extra stuff to make things simpler, but there was still a dependency on this file. https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... tools/migration/lib/src/path.dart:146: Path join(Path further) { On 2017/07/24 18:24:51, jcollins wrote: > Ordinarily, methods named "join" take an Iterable as a parameter, so calling > this method join makes uses of the class harder to read. Can we either make > this take an iterable or call it something else? Maybe joinWith? To be honest, this is only here because status_file.dart depends on it. I didn't write this, so I don't really know much about it myself. https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... tools/migration/lib/src/path.dart:154: if (hasTrailingSeparator) { On 2017/07/24 18:24:51, jcollins wrote: > This looks very much like append() from here out See above. https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/sta... File tools/migration/lib/src/status_file.dart (right): https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/sta... tools/migration/lib/src/status_file.dart:9: /// Matches the header that begins a new section, like: On 2017/07/24 18:24:51, jcollins wrote: > There should probably be a TODO here to merge this code somehow with whatever > code reads these files "for real". Do you mean with tools/testing/dart/status_file.dart?
I agree totally that the status file monkeying is tedious and this is a good step in the right direction. :) We should figure out how to reuse the existing status file code without copy/pasting it first, though. I'll start a thread on that. https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... File tools/migration/bin/migrate_batch.dart (right): https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:47: print("Preparing to migrate tests from ${bold(first)} to ${bold(last)}..."); This is because the scan process is a lot longer with your change, right? Let's see if we can fix that directly... https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:111: statusFileEntries += printStatusFileEntries(tests[i]); Instead of repeatedly concatenating strings, it's a little faster and more idiomatic to use a StringBuffer. Something like: var buffer = new StringBuffer(); for (var i = startIndex; i < endIndex; i++) { writeStatusFileEntries(buffer, tests[i]); } new File("statuses.migration").writeAsStringSync(buffer.toString()); void writeStatusFileEntries(StringBuffer buffer, Fork test) { if (test.onePath != null && !test.oneStatusFile.isEmpty) { buffer.writeln("Status file entries for: ${test.onePath}"); ... } } https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:184: var statusFile = getStatusFileEntries(fromStatus, testName); This call does two things: 1. It reads the status file associated with this test. 2. It figures out which sections in the status file apply to the test. Step 1 hits the file system (i.e. is slow) and does the same work for every test in the directory. We shouldn't do that for each test. Instead, how about: 1. After all the tests are scanned, do a separate scan to read all of the status files in each test directory. Stuff the results in something like a Map<String, StatusFile> where the keys are the file names. Or maybe Map<String, List<StatusFile>> where the keys are the test directories and the values are the lists of status files in each directory (since a directory may have more than one). 2. Then, for each test being migrated, go through that map and find the relevant status files and sections. That way, any given status file is only read from disc once (similar to what we do for the tests themselves). https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:213: final filteredStatusFile = new StatusFile(path); Style nit: Elsewhere, I tend to use "var" even for locals that aren't modified. I don't find "final" adds too much value since most methods are pretty short anyway. If you do really like using final, I'll note that you can use it for the loop variables below as well. (A for-in loop in Dart doesn't mutate the loop variable, it creates a new one each iteration. The difference is subtle, but user-visible if you close over a loop variable in a closure.) https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:268: StatusFile oneStatusFile; Unfortunately, to make things more annoying, there may be more than one status file for a given directory. For example, language/ has: language.status language_dart2js.status language_kernel.status Ideally, it would find all of those. You could probably just look in the directory for every "*.status" file. https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... File tools/migration/lib/src/path.dart (right): https://codereview.chromium.org/2987633002/diff/1/tools/migration/lib/src/pat... tools/migration/lib/src/path.dart:5: library legacy_path; On 2017/07/24 19:16:04, bkonyi wrote: > On 2017/07/24 18:24:51, jcollins wrote: > > Did this module get copied from somewhere else? > > Yes, this is from the test runner code. I grabbed the status_file.dart from > there and removed a lot of the extra stuff to make things simpler, but there was > still a dependency on this file. Oof, this is tricky. We don't want to copy/paste files inside the repo since it will make our future lives hell. (Note that our current task is exactly related to unforking a set of copy/pasted files...) However, test.dart is not designed to make it easy for you to reuse any of its code. This is somewhat deliberate: we want it to have as few dependencies as possible so that our testing infrastructure isn't itself reliant on all of the things it exists to test. But I'm not convinced that we should be so dogmatic. I'll start a thread with the relevant folks and you to see how we want to handle this.
I've updated the code to use pkg:status_file and now check for the relevant entries in a more intelligent way. As a result, this CL depends on https://codereview.chromium.org/2984203002/ landing. PTAL! https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... File tools/migration/bin/migrate_batch.dart (right): https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:47: print("Preparing to migrate tests from ${bold(first)} to ${bold(last)}..."); On 2017/07/24 21:47:47, Bob Nystrom wrote: > This is because the scan process is a lot longer with your change, right? Let's > see if we can fix that directly... Oh yeah, this is here because I'm really not doing things as efficiently as I could be when scanning through the status files. It's not terribly slow, but it's noticeable for sure. I've fixed this now. https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:111: statusFileEntries += printStatusFileEntries(tests[i]); On 2017/07/24 21:47:47, Bob Nystrom wrote: > Instead of repeatedly concatenating strings, it's a little faster and more > idiomatic to use a StringBuffer. Something like: > > var buffer = new StringBuffer(); > for (var i = startIndex; i < endIndex; i++) { > writeStatusFileEntries(buffer, tests[i]); > } > > new File("statuses.migration").writeAsStringSync(buffer.toString()); > > void writeStatusFileEntries(StringBuffer buffer, Fork test) { > if (test.onePath != null && !test.oneStatusFile.isEmpty) { > buffer.writeln("Status file entries for: ${test.onePath}"); > ... > } > } > Right, that makes sense. Done. https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:184: var statusFile = getStatusFileEntries(fromStatus, testName); On 2017/07/24 21:47:47, Bob Nystrom wrote: > This call does two things: > > 1. It reads the status file associated with this test. > 2. It figures out which sections in the status file apply to the test. > > Step 1 hits the file system (i.e. is slow) and does the same work for every test > in the directory. We shouldn't do that for each test. > > Instead, how about: > > 1. After all the tests are scanned, do a separate scan to read all of the status > files in each test directory. Stuff the results in something like a Map<String, > StatusFile> where the keys are the file names. Or maybe Map<String, > List<StatusFile>> where the keys are the test directories and the values are the > lists of status files in each directory (since a directory may have more than > one). > > 2. Then, for each test being migrated, go through that map and find the relevant > status files and sections. > > That way, any given status file is only read from disc once (similar to what we > do for the tests themselves). > Done. https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:213: final filteredStatusFile = new StatusFile(path); On 2017/07/24 21:47:47, Bob Nystrom wrote: > Style nit: Elsewhere, I tend to use "var" even for locals that aren't modified. > I don't find "final" adds too much value since most methods are pretty short > anyway. > > If you do really like using final, I'll note that you can use it for the loop > variables below as well. (A for-in loop in Dart doesn't mutate the loop > variable, it creates a new one each iteration. The difference is subtle, but > user-visible if you close over a loop variable in a closure.) Hm, that's interesting. I've heard different opinions in the var vs final debate. Do we have something about this in the official style guide? https://codereview.chromium.org/2987633002/diff/1/tools/migration/bin/migrate... tools/migration/bin/migrate_batch.dart:268: StatusFile oneStatusFile; On 2017/07/24 21:47:47, Bob Nystrom wrote: > Unfortunately, to make things more annoying, there may be more than one status > file for a given directory. For example, language/ has: > > language.status > language_dart2js.status > language_kernel.status > > Ideally, it would find all of those. You could probably just look in the > directory for every "*.status" file. Ah, I didn't realize that. Fixed.
Much better! https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... File tools/migration/bin/migrate_batch.dart (right): https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:120: var statusFileEntries = new StringBuffer(); Instead of creating a new StringBuffer in each call to this whose results are added to the outer StringBuffer in main(), it will be faster to just pass that one StringBuffer in and keep adding to it. (Not that this code is super perf critical, but it's a good habit to be in and doesn't make the code notably more complex.) https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:127: StatusSection currentSection = null; "= null" isn't needed. https://www.dartlang.org/guides/language/effective-dart/usage#dont-explicitly... https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:144: statusFileEntries.writeln(filteredStatusFile.toString()); ".toString()" isn't needed. StringBuffer.writeln() accepts any object and automatically calls toString() on it for you. https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:215: var statusFiles = <String, List<StatusFile>>{}; Looks like my suggestion was wrong and you don't actually need to group the status files by anything. Maybe discard this and just build one big List<StatusFile>?
https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... File tools/migration/bin/migrate_batch.dart (right): https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:120: var statusFileEntries = new StringBuffer(); On 2017/07/25 23:48:21, Bob Nystrom wrote: > Instead of creating a new StringBuffer in each call to this whose results are > added to the outer StringBuffer in main(), it will be faster to just pass that > one StringBuffer in and keep adding to it. (Not that this code is super perf > critical, but it's a good habit to be in and doesn't make the code notably more > complex.) Done. https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:127: StatusSection currentSection = null; On 2017/07/25 23:48:21, Bob Nystrom wrote: > "= null" isn't needed. > > https://www.dartlang.org/guides/language/effective-dart/usage#dont-explicitly... Done. https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:144: statusFileEntries.writeln(filteredStatusFile.toString()); On 2017/07/25 23:48:21, Bob Nystrom wrote: > ".toString()" isn't needed. StringBuffer.writeln() accepts any object and > automatically calls toString() on it for you. Done. https://codereview.chromium.org/2987633002/diff/20001/tools/migration/bin/mig... tools/migration/bin/migrate_batch.dart:215: var statusFiles = <String, List<StatusFile>>{}; On 2017/07/25 23:48:21, Bob Nystrom wrote: > Looks like my suggestion was wrong and you don't actually need to group the > status files by anything. Maybe discard this and just build one big > List<StatusFile>? Done.
LGTM!
Description was changed from ========== Added functionality to dump relevant status file entries to a single file to make it easier to see which tests have which status file entries. Future work: remove entries from original status files. BUG= ========== to ========== Added functionality to dump relevant status file entries to a single file to make it easier to see which tests have which status file entries. Future work: remove entries from original status files. BUG= R=jcollins@google.com, rnystrom@google.com Committed: https://github.com/dart-lang/sdk/commit/caf87da9ed218f45cd68ca2f52253c8b913eb53a ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as caf87da9ed218f45cd68ca2f52253c8b913eb53a (presubmit successful). |