CP-3183 Create dart_dev task to allow for tasks to run con-currently by jayudey-wf · Pull Request #199 · Workiva/dart_dev

jayudey-wf · 2016-12-16T16:59:12Z

Issue

There is no way to easily run dart tasks asynchronously.

Changes

Source:

Created a task to allow for a user to specify dart commands that they wish to run in parallel.

Tests:

added

Areas of Regression

n/a (new tasks was added)

Testing

passing CI
install into this branch into a repo and ensure that the tasks complete as expected (I tried doing this on travis CI for dart_dev and I don't think the instance could handle running the tasks)

Code Review

@Workiva/web-platform-pp

maxwellpeterson-wf

This looks awesome!
Just a few small nits. I can't wait to use this.

maxwellpeterson-wf · 2017-01-12T05:30:57Z

lib/src/tasks/task_runner/api.dart

+Future<TaskRunner> runTasks({List<String> tasksToRun}) async {
+  var taskGroup = new TaskGroup(tasksToRun);
+  await taskGroup.startTaskGroup();
+  taskGroup.taskGroupOutput();


Consider renaming this method to output() to reduce redundancy in the name

maxwellpeterson-wf · 2017-01-12T05:32:55Z

lib/src/tasks/task_runner/api.dart

+  bool successful;
+
+  TaskRunner(int exitCode, String this.stdout, String this.stderr) {
+    exitCode == 0 ? this.successful = true : this.successful = false;


This can be simplified to:

this.successful = exitCode == 0;

maxwellpeterson-wf · 2017-01-12T05:39:01Z

lib/src/tasks/task_runner/api.dart

+  }
+
+  /// Begin each subtask and wait for completion
+  startTaskGroup() async {


Consider renaming this method to start

maxwellpeterson-wf · 2017-01-12T05:41:04Z

lib/src/tasks/task_runner/config.dart

+
+const List<String> defaultTasks = const [
+  'pub run dart_dev format --check',
+  'pub run dart_dev analyze'


Should we add test to the default list?

georgelesica-wf · 2017-01-12T16:23:19Z

README.md

 - **Applying a License to Source Files:** copies a LICENSE file to all applicable files.
 - **Generate a test runner file:** that allows for faster test execution.
 - **Running dart unit tests on Sauce Labs** compiles dart unit tests that can be run in the browser and executes them on various platforms using Sauce Labs.
+- **Running concurrent dart commands** allows for continuous integration systems to run concurrently rather then serially.


"allows continuous integration..."

georgelesica-wf · 2017-01-12T16:26:03Z

README.md

 ddev format
 ddev gen-test-runner
 ddev saucelabs
+ddev task-runner


#nit the other task names are imperative, "format", "test". Obviously there are exceptions ("saucelabs"). I prefer the former. Could we change the name to "run-task" or "run-async" or something?

Given the description above it might actually be nice to name the command something to do with CI. Liked run-ci or something, to make it clear that this isn't meant to be used interactively.

I think I could go either way on this update. I'll let the other reviewers weigh in since it could theoretically be used outside of CI (even though I agree that is the most common use case)

georgelesica-wf · 2017-01-12T16:37:04Z

lib/src/tasks/task_runner/api.dart

+
+  TaskProcess taskProcess;
+
+  SubTask(String task) {


SubTask(this.command);

georgelesica-wf · 2017-01-12T16:37:49Z

lib/src/tasks/task_runner/api.dart

+
+class SubTask {
+  /// Process to be executed
+  String command;


I think you can mark this final.

codecov-io · 2017-01-12T23:25:37Z

Current coverage is 23.56% (diff: 100%)

Merging #199 into master will not change coverage

@@             master       #199   diff @@
==========================================
  Files             7          7          
  Lines           157        157          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits             37         37          
  Misses          120        120          
  Partials          0          0

Powered by Codecov. Last update 6de67f6...c8c5d50

maxwellpeterson-wf · 2017-01-13T06:25:14Z

+1

bencampbell-wf

I had a few questions and some nits.

bencampbell-wf · 2017-01-13T18:18:26Z

README.md

 - **Applying a License to Source Files:** copies a LICENSE file to all applicable files.
 - **Generate a test runner file:** that allows for faster test execution.
 - **Running dart unit tests on Sauce Labs** compiles dart unit tests that can be run in the browser and executes them on various platforms using Sauce Labs.
+- **Running concurrent dart commands** allows continuous integration systems to run concurrently rather then serially.


🌵 The upper elements in the use a colon, the bottom two do not.

bencampbell-wf · 2017-01-13T18:25:57Z

test/integration/task_runner_test.dart

+    }
+  });
+
+  await process.done;


❓ Can you explain the necessity of this line? This appears to the be only consumer of the final Future value on TaskRunner Class .

that is waiting for the TaskProcess that is checking for the task that is executing the task runner https://github.com/Workiva/dart_dev/pull/199/files#diff-f47564d08708b67413d675e327f15302R34

bencampbell-wf · 2017-01-13T18:27:14Z

lib/src/tasks/task_runner/api.dart

+  /// TaskGroup stderr
+  String taskGroupStderr = '';
+
+  TaskGroup(List<String> this.subTaskCommands) {


I don't think you need the type here if you are referring to the class member subTaskCommands.

bencampbell-wf · 2017-01-13T18:27:54Z

lib/src/tasks/task_runner/api.dart

+
+class TaskGroup {
+  /// List of the individual subtasks executed
+  List<String> subTaskCommands = [];


🌵 List<String> subTaskCommands = <String>[];

bencampbell-wf · 2017-01-13T18:28:14Z

lib/src/tasks/task_runner/api.dart

+  List<String> subTaskCommands = [];
+
+  /// List of the subtasks making up the TaskGroup
+  List<SubTask> subTasks = [];


🌵 List<SubTask> subTasks = <SubTask>[];

bencampbell-wf · 2017-01-13T18:36:42Z

lib/src/tasks/task_runner/api.dart

+  }
+
+  /// Begin each subtask and wait for completion
+  start() async {


🌵 `void start() async {

Future start() async ;)

bencampbell-wf · 2017-01-13T18:39:56Z

lib/src/tasks/task_runner/api.dart

+  /// Determine if subtasks completed successfully
+  Future<int> checkExitCodes() async {
+    for (SubTask task in subTasks) {
+      if (await task.taskProcess.exitCode != 0) {


🔮 Is it good practice to include awaits in a if statement?

IMO I don't see a problem with it ¯_(ツ)_/¯

I've always kind of avoided the practice and rarely see it done elsewhere. Its no biggie, I thought I would just ask.

bencampbell-wf · 2017-01-13T18:41:43Z

test/integration/task_runner_test.dart

+  var args = ['run', 'dart_dev', 'task-runner'];
+  TaskProcess process =
+      new TaskProcess('pub', args, workingDirectory: projectPath);
+  List<String> failedTasks = [];


🌵 List<String> failedTasks = <String>[];

bencampbell-wf · 2017-01-13T18:41:57Z

test/integration/task_runner_test.dart

+  TaskProcess process =
+      new TaskProcess('pub', args, workingDirectory: projectPath);
+  List<String> failedTasks = [];
+  List<String> successfulTasks = [];


🌵 List<String> successfulTasks = <String>[];

bencampbell-wf · 2017-01-13T18:44:36Z

lib/src/tasks/task_runner/cli.dart

+
+    TaskRunner task = await runTasks(tasksToRun: tasksToRun);
+
+    reporter.logGroup('Tasks run: \'${tasksToRun.join('\', \'')}\'',


😄 So much escaping!

maxwellpeterson-wf · 2017-01-18T19:49:21Z

+1

todbachman-wf · 2017-01-18T22:24:49Z

README.md

+        <tr>
+            <td><code>tasksToRun</code></td>
+            <td><code>List&lt;String&gt;</code></td>
+            <td><code>['pub run dart_dev format --check','pub run dart_dev analyze']</code></td>


Any reason not to default it to run the unit tests too?

maxwellpeterson-wf · 2017-01-18T22:29:21Z

+1

evanweible-wf · 2017-01-18T22:39:03Z

lib/src/tasks/task_runner/api.dart

+
+import 'package:dart_dev/src/tasks/task.dart';
+
+Future<TaskRunner> runTasks({List<String> tasksToRun}) async {


This param shouldn't be optional since you're not defaulting it to anything in that scenario.

evanweible-wf · 2017-01-18T22:39:39Z

README.md

 ddev format
 ddev gen-test-runner
 ddev saucelabs
+ddev run-ci


The command is task-runner

evanweible-wf · 2017-01-18T22:39:44Z

README.md

 pub run dart_dev format
 pub run dart_dev gen-test-runner
 pub run dart_dev saucelabs
+pub run dart_dev run-ci


robbecker-wf · 2017-01-19T15:36:41Z

README.md

+        <tr>
+            <td><code>tasksToRun</code></td>
+            <td><code>List&lt;String&gt;</code></td>
+            <td><code>['pub run dart_dev format --check','pub run dart_dev analyze','pub run dart_dev test']</code></td>


you might want to include examples of running other things besides dart_dev (if it indeed supports that, which it looks like it does)

robbecker-wf · 2017-01-19T15:39:52Z

lib/src/tasks/task_runner/api.dart

+  final String command;
+
+  /// A string consisting of all the subtask's output
+  String taskOutput = '';


does buffering the output in a string mean we can't display output from subtasks as it happens but instead all at once once it succeeds or fails? No change requested here, just wondering.

yeah, it comes all at once

robbecker-wf · 2017-01-19T15:44:20Z

test_fixtures/task_runner/failing_tasks/lib/main.dart

+List longList = [
+  'one',
+  'two',
+  'three', 'four',


total #nit why is four not on its own line?

this is to cause the task to fail to verify that a failed task ran fails the parent task

robbecker-wf · 2017-01-19T15:45:53Z

+1 with a question and an optional nit. FWIW I'm ok with the name task-runner.

maxwellpeterson-wf · 2017-01-19T17:19:38Z

+1

maxwellpeterson-wf · 2017-01-20T03:07:15Z

+1

evanweible-wf · 2017-01-23T17:08:22Z

+1

maxwellpeterson-wf · 2017-01-25T17:02:50Z

+10

Verified it works in wdesk_sdk: https://ci.webfilings.com/build/775440

maxwellpeterson-wf · 2017-01-25T17:04:22Z

QA +1

Merging into master.

create dart_dev task to allow for tasks to run con-currently

4f891a1

rmconsole-wf changed the title ~~Create dart_dev task to allow for tasks to run con-currently~~ CP-3183 Create dart_dev task to allow for tasks to run con-currently Dec 16, 2016

maxwellpeterson-wf approved these changes Jan 12, 2017

View reviewed changes

update function names, simplify success check, add test task to config

e867d66

jayudey-wf force-pushed the task-runner branch from 53edd93 to e867d66 Compare January 12, 2017 15:39

georgelesica-wf reviewed Jan 12, 2017

View reviewed changes

code review comments

5328588

bencampbell-wf reviewed Jan 13, 2017

View reviewed changes

adjust readme, add more typing

44db530

todbachman-wf reviewed Jan 18, 2017

View reviewed changes

update readme to reflect config

c8c5d50

evanweible-wf reviewed Jan 18, 2017

View reviewed changes

update parameter, more readme updates

5594c58

robbecker-wf reviewed Jan 19, 2017

View reviewed changes

pin travis

2968e4a

maxwellpeterson-wf merged commit 5c429fe into master Jan 25, 2017

maxwellpeterson-wf deleted the task-runner branch January 25, 2017 17:04

rmconsole-wf mentioned this pull request Jan 25, 2017

CP-3325 Release dart_dev 1.7.0 #207

Merged


		TaskRunner task = await runTasks(tasksToRun: tasksToRun);

		reporter.logGroup('Tasks run: \'${tasksToRun.join('\', \'')}\'',


		import 'package:dart_dev/src/tasks/task.dart';

		Future<TaskRunner> runTasks({List<String> tasksToRun}) async {

Conversation

jayudey-wf commented Dec 16, 2016 • edited by trentgrover-wf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Changes

Areas of Regression

Testing

Code Review

Uh oh!

maxwellpeterson-wf 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!

codecov-io commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 23.56% (diff: 100%)

Uh oh!

maxwellpeterson-wf commented Jan 13, 2017

Uh oh!

bencampbell-wf 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!

maxwellpeterson-wf commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxwellpeterson-wf commented Jan 18, 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

jayudey-wf commented Dec 16, 2016 •

edited by trentgrover-wf

Loading

codecov-io commented Jan 12, 2017 •

edited

Loading

robbecker-wf Jan 19, 2017 •

edited

Loading