Migrate status file entries when migrating files. Yay!

Migrate status file entries when migrating files. Yay!

This looks at all the files in the batch being migrated. It goes through
the status files in the 1.0 and strong directories. If it sees an entry
that matches that file, it adds it to the resulting "_2" status file.

It handles adding entries to an existing section if already present in
the new status file. Otherwise, it adds a new section at the end with
the entries.

R=bkonyi@google.com

Committed: https://github.com/dart-lang/sdk/commit/385b88f2894d9d5482ae27c0ea052e943c280750

[Bob Nystrom, 2017-07-27 21:40:03]

rnystrom@google.com changed reviewers:
+ bkonyi@google.com

[bkonyi, 2017-07-27 22:02:01]

Overall, this looks good! I did notice one issue caused by a recent updated to
migration.dart where the status file entries in statuses.migration would
sometimes contain false positives (status file entries for tests from other test
directories with the same name, and entries for tests that contain the name of
the start/end tests, even if they're not in the test range). Have you double
checked that you're not doing status file migrations for tests that aren't in
the block or aren't even in the right directory?

https://codereview.chromium.org/2989033002/diff/1/tools/migration/bin/migrate...
File tools/migration/bin/migrate_batch.dart (right):

https://codereview.chromium.org/2989033002/diff/1/tools/migration/bin/migrate...
tools/migration/bin/migrate_batch.dart:57: print("Migrating
${bold(tests.length)} test$s from ${bold(first)} "
Did you mean to put 'test$s'?

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/edi...
File tools/migration/lib/src/editable_status_file.dart (right):

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/edi...
tools/migration/lib/src/editable_status_file.dart:21: class EditableStatusFile {
Do we maybe want to add this to package:status_file? It could be useful in other
tooling that can be used to manage status files.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
File tools/migration/lib/src/migrate_statuses.dart (right):

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:24: // TODO(rnystrom): Should this
log any output?
I'm not sure if it would be worth still outputting a file like we were before,
but it might make it easier to understand what status file entries were migrated
and help track down entries that cause different failures in Dart 2.0 more
quickly instead of searching for the test name in all the status files.
Thoughts?

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:62: // TODO(rnystrom): For now, we
don't support entries in the initial
I haven't run into this case yet myself, and I think it's unlikely that someone
has made a status file entry that doesn't have a condition associated with it.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:91: // would be nice to delete the
section header too.
Agreed.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:110: // TODO(bob): Handle adding
stuff to the empty section.
// TODO(rnystrom): ...

[Bob Nystrom, 2017-07-27 22:58:19]

Thanks!

https://codereview.chromium.org/2989033002/diff/1/tools/migration/bin/migrate...
File tools/migration/bin/migrate_batch.dart (right):

https://codereview.chromium.org/2989033002/diff/1/tools/migration/bin/migrate...
tools/migration/bin/migrate_batch.dart:57: print("Migrating
${bold(tests.length)} test$s from ${bold(first)} "
On 2017/07/27 22:02:00, bkonyi wrote:
> Did you mean to put 'test$s'?

Yup. See the "s" variable above. It will print "tests" if there is more than one
and "test" if only one.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/edi...
File tools/migration/lib/src/editable_status_file.dart (right):

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/edi...
tools/migration/lib/src/editable_status_file.dart:21: class EditableStatusFile {
On 2017/07/27 22:02:00, bkonyi wrote:
> Do we maybe want to add this to package:status_file? It could be useful in
other
> tooling that can be used to manage status files.

Eventually, maybe. But it's pretty hacky so I didn't feel like it was worth
sticking in the actual package. tools/migration/ is held to a lower standard
since it will get deleted once the migration is done.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
File tools/migration/lib/src/migrate_statuses.dart (right):

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:24: // TODO(rnystrom): Should this
log any output?
On 2017/07/27 22:02:00, bkonyi wrote:
> I'm not sure if it would be worth still outputting a file like we were before,
> but it might make it easier to understand what status file entries were
migrated
> and help track down entries that cause different failures in Dart 2.0 more
> quickly instead of searching for the test name in all the status files.
> Thoughts?

You'll see the actual diff of the changes when you go to commit it in Git, so I
figured that was probably as clear as you can get.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:62: // TODO(rnystrom): For now, we
don't support entries in the initial
On 2017/07/27 22:02:01, bkonyi wrote:
> I haven't run into this case yet myself, and I think it's unlikely that
someone
> has made a status file entry that doesn't have a condition associated with it.

There are definitely ones like that, but not too many, I don't think.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:91: // would be nice to delete the
section header too.
On 2017/07/27 22:02:00, bkonyi wrote:
> Agreed.

Acknowledged.

https://codereview.chromium.org/2989033002/diff/1/tools/migration/lib/src/mig...
tools/migration/lib/src/migrate_statuses.dart:110: // TODO(bob): Handle adding
stuff to the empty section.
On 2017/07/27 22:02:01, bkonyi wrote:
> // TODO(rnystrom): ...

Oops, don't need that anymore. Deleted.

[bkonyi, 2017-07-28 17:43:40]

lgtm

[Bob Nystrom, 2017-07-28 18:10:34]

Description was changed from

==========
Migrate status file entries when migrating files. Yay!

This looks at all the files in the batch being migrated. It goes through
the status files in the 1.0 and strong directories. If it sees an entry
that matches that file, it adds it to the resulting "_2" status file.

It handles adding entries to an existing section if already present in
the new status file. Otherwise, it adds a new section at the end with
the entries.
==========

to

==========
Migrate status file entries when migrating files. Yay!

This looks at all the files in the batch being migrated. It goes through
the status files in the 1.0 and strong directories. If it sees an entry
that matches that file, it adds it to the resulting "_2" status file.

It handles adding entries to an existing section if already present in
the new status file. Otherwise, it adds a new section at the end with
the entries.

R=bkonyi@google.com

Committed:
https://github.com/dart-lang/sdk/commit/385b88f2894d9d5482ae27c0ea052e943c280750
==========

[Bob Nystrom, 2017-07-28 18:10:35]

Committed patchset #2 (id:20001) manually as
385b88f2894d9d5482ae27c0ea052e943c280750 (presubmit successful).