flutter test: add --machine flag #10520

skybrian · 2017-06-06T01:29:16Z

Currently this just prints the observatory URL as a JSON event.
Refactored the code to make this fit in.

Hixie · 2017-06-06T01:33:06Z

packages/flutter_tools/lib/src/commands/test.dart

nit: spaces around named arguments

This contradicts dartfmt?

flutter repo doesn't adhere to dartfmt since it doesn't meet our needs. @Hixie has a list somewhere of all our wishes in a formatter...

Huh. Okay, but what special needs can there be for the flutter_tools package? So far it seems to be just regular Dart VM code.

Hixie · 2017-06-06T01:33:54Z

@tvolkert would be a better reviewer.

devoncarew

lgtm for my part w/ the indicated changes, but would also wait on Todd's review.

devoncarew · 2017-06-06T05:30:39Z

packages/flutter_tools/lib/src/test/event_printer.dart

This file will need a copyright statement.

I'll use "Chromium Authors" to match the others, but wouldn't "Flutter Authors" be better?

"Chromium Authors" is a legal term; I think we have some license deps to free ourselves of first before we can become the "Flutter Authors"... @sethladd would know the full story.

devoncarew · 2017-06-06T05:32:31Z

packages/flutter_tools/lib/src/test/event_printer.dart

Name the 2nd arg params to parallel the map assignment below?

devoncarew · 2017-06-06T05:34:45Z

packages/flutter_tools/lib/src/test/runner.dart

same re: copyright

devoncarew · 2017-06-06T05:35:32Z

packages/flutter_tools/lib/src/test/watcher.dart

file copyright

devoncarew · 2017-06-06T05:38:35Z

packages/flutter_tools/lib/src/commands/test.dart

also, negatable: false?

tvolkert

Note: coverage doesn't run on Travis for PRs. The best way to make sure this doesn't introduce any regressions is to roll it locally into google's internal system and run all affected tests (the roll script supports pointing to a local repo).

tvolkert · 2017-06-06T06:30:29Z

packages/flutter_tools/lib/src/test/runner.dart

Nit: format:

... runTests( Type1 arg1, { Type2 arg2, Type3 arg3, }) {

tvolkert · 2017-06-06T06:39:41Z

packages/flutter_tools/lib/src/test/runner.dart

No curly braces around single-line if statements

tvolkert · 2017-06-06T06:41:53Z

packages/flutter_tools/lib/src/test/runner.dart

It's weird to test a boolean value for null, especially when it has a default value. Do you mean enableObservatory ?? false?

Huh. I must have copied that from somewhere else. Fixed.

tvolkert · 2017-06-06T06:49:10Z

packages/flutter_tools/lib/src/test/runner.dart

This was originally added for coverage only, and I wanna make sure we're not conflating it with enableObservatory -- do we need this if startPaused is true?

It looks like it should make no difference, because for startPaused, you are only allowed to pass in one test file. (That is, asssuming that a test file corresponds to a "test suite" in package:test.)

Also, it seems like debugging concurrently running tests would be unnecessarily confusing?

tvolkert · 2017-06-06T06:52:44Z

packages/flutter_tools/lib/src/test/runner.dart

No curly braces

tvolkert · 2017-06-06T06:53:00Z

packages/flutter_tools/lib/src/test/runner.dart

No curly barces

tvolkert · 2017-06-06T06:54:17Z

packages/flutter_tools/lib/src/test/runner.dart

It looks like coverage has been dropped - we need to pass collector.

Passed in as watcher.

tvolkert · 2017-06-06T06:56:28Z

packages/flutter_tools/lib/src/test/flutter_platform.dart

You're no longer plumbing collector through

It is being plumbed through as watcher. (CoverageCollector now implements TestWatcher.)

Hixie · 2017-06-07T19:31:24Z

packages/flutter_tools/lib/src/test/flutter_platform.dart

nit: no braces

Okay, but Effective Dart says the opposite.

https://www.dartlang.org/guides/language/effective-dart/style#do-use-curly-braces-for-all-flow-control-structures

devoncarew · 2017-06-07T19:33:10Z

(fyi, @skybrian is out for a few days - he likely won't revisit this PR until next week)

Currently this just prints the observatory URL as a JSON event. Refactored the code to make this fit in.

Hixie · 2017-06-12T19:24:04Z

I'm going to close this PR since @skybrian is out for a while. Please don't hesitate to reopen when it's ready for further review.

tvolkert

LGTM with comments

tvolkert · 2017-06-13T02:54:34Z

packages/flutter_tools/lib/src/test/watcher.dart

+import '../base/io.dart' show Process;
+
+/// Callbacks for reporting progress while running tests.
+class TestWatcher {


It could be, but technically it's okay to use the base class as a no-op implementation, so I'll leave it as is.

tvolkert · 2017-06-13T03:04:11Z

packages/flutter_tools/lib/src/test/runner.dart

+  );
+
+  // Set the package path used for child processes.
+  // TODO: why is this global? Move to installHook?


Nit: format of TODO comments is TODO(username): ...

Done. (I guess nobody knows the answer?)

tvolkert · 2017-06-13T03:04:45Z

packages/flutter_tools/lib/src/test/runner.dart

+    await test.main(testArgs);
+
+    // test.main() sets dart:io's exitCode global.
+    // TODO: restore previous value?


Nit: format of TODO comments is TODO(username): ...

tvolkert · 2017-06-13T03:14:35Z

packages/flutter_tools/lib/src/commands/test.dart

    }

-    if (result != 0)
+    if (result != 0) {


Nit: no braces

Hixie · 2017-06-13T23:58:12Z

For future archeologists: this broke coverage; fixes started in #10672.

skybrian requested review from Hixie and devoncarew June 6, 2017 01:29

googlebot added the cla: yes label Jun 6, 2017

Hixie reviewed Jun 6, 2017

View reviewed changes

skybrian requested a review from tvolkert June 6, 2017 04:20

devoncarew approved these changes Jun 6, 2017

View reviewed changes

tvolkert suggested changes Jun 6, 2017

View reviewed changes

Hixie reviewed Jun 7, 2017

View reviewed changes

flutter test: add --machine flag

9d118b6

Currently this just prints the observatory URL as a JSON event. Refactored the code to make this fit in.

Hixie closed this Jun 12, 2017

devoncarew mentioned this pull request Jun 12, 2017

Run flutter unit tests (flutter test) flutter/flutter-intellij#66

Closed

skybrian reopened this Jun 12, 2017

Brian Slesinsky added 2 commits June 12, 2017 17:17

post-review changes

e2c6e6b

satisfy lint

3465a88

tvolkert approved these changes Jun 13, 2017

View reviewed changes

more post-review nits

2af7cef

tvolkert approved these changes Jun 13, 2017

View reviewed changes

skybrian merged commit 3528cd6 into flutter:master Jun 13, 2017

skybrian deleted the debug branch June 15, 2017 20:44

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021

flutter test: add --machine flag #10520

flutter test: add --machine flag #10520

Uh oh!

Conversation

skybrian commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jun 6, 2017

Uh oh!

devoncarew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!