AF-2971 Dart 2 compatibility #137

robbecker-wf · 2018-10-24T03:28:07Z

Overview

Updates are needed to use this library under Dart 2

Changes

updated analysis options and renamed to the proper name
updated dependencies and dart sdk ranges in pubspec.yaml
unit tests fixed for dart 2.
fixed some type errors in the examples
CI now enforces dartfmt under dart 2

Testing

CI passes under dart 1 and dart 2

aviary3-wk · 2018-10-24T03:28:22Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on HipChat: InfoSec Forum.

- adjust a type error - work around sync-async issues - shorten dockerfile run since everything happens in Travis CI

robbecker-wf · 2018-10-27T18:49:52Z

example/panel/modules/sample_tracer.dart

@@ -32,8 +32,7 @@ class SampleSpan implements Span {
    this.references,
    DateTime startTime,
    Map<String, dynamic> tags,
-  })
-      : this.startTime = startTime ?? new DateTime.now(),
+  })  : this.startTime = startTime ?? new DateTime.now(),


dart 2 formatter change

robbecker-wf · 2018-10-27T18:51:36Z

lib/src/lifecycle_module.dart

@@ -594,7 +594,7 @@ abstract class LifecycleModule extends SimpleModule with Disposable {
          ]);
    }

-    Future pendingTransition;
+    Future<Null> pendingTransition;


changed to match the type signature of _suspend

this is internal and not a public api change

robbecker-wf · 2018-10-27T18:51:49Z

lib/src/lifecycle_module.dart

@@ -661,7 +661,7 @@ abstract class LifecycleModule extends SimpleModule with Disposable {
          allowedStates: [LifecycleState.suspended, LifecycleState.suspending]);
    }

-    Future pendingTransition;
+    Future<Null> pendingTransition;


changed to match the type signature of _resume

also internal only. no public api change.

robbecker-wf · 2018-10-27T18:55:49Z

test/lifecycle_module_test.dart

@@ -280,6 +282,8 @@ void expectInLifecycleState(LifecycleModule module, LifecycleState state) {
 }

 Future<Null> gotoState(LifecycleModule module, LifecycleState state) async {
+  // wait for next event loop. fixes sync-async in Dart 2
+  await new Future.value(null);


@Workiva/app-frameworks want to take a look at these closely to determine if we're going to have a sync-async problem with lifecycle module or if it's just the tests and mocks?

well, goto state is only used internally in tests, so I'm not too concerned about this one.

codecov-io · 2018-10-28T18:44:47Z

Codecov Report

Merging #137 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #137   +/-   ##
=======================================
  Coverage   96.82%   96.82%           
=======================================
  Files           4        4           
  Lines         346      346           
=======================================
  Hits          335      335           
  Misses         11       11

Impacted Files	Coverage Δ
lib/src/lifecycle_module.dart	`96.65% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3536c82...502dc3f. Read the comment docs.

robbecker-wf · 2018-10-28T18:53:37Z

Dockerfile

@@ -1,43 +1,6 @@
 FROM google/dart:1.24.3 as build
-


Just let Travis CI run all the tests.

robbecker-wf · 2018-10-29T23:01:00Z

test/lifecycle_module_test.dart

@@ -149,6 +149,7 @@ class TestLifecycleModule extends LifecycleModule {
  @override
  @protected
  Future<Null> onWillUnloadChildModule(LifecycleModule module) async {
+    await new Future.value(null);


This spot is a little more concerning because this may mean that it's possible for other consumers that are overriding one of these lifecycle methods may get a change in how lifecycle events are ordered due to sync-async. I think we can go ahead with this PR as is, but be aware that there may be a recommended migration step of putting a line like this in each overriden module lifecycle method.

Without this await here .. the order that events are added to the tracking list of events is different than what the tests expect the order to be. This looks like:

00:05 +216 -1: [VM] test/lifecycle_module_test.dart: LifecycleModule with globalTracer with children unload should wait for in-progress child module loads [E] Expected: [ 'onWillLoadChildModule', 'willLoadChildModule', 'onShouldUnload', 'willUnload', 'onDidLoadChildModule', 'didLoadChildModule', 'onWillUnloadChildModule', 'willUnloadChildModule', 'onDidUnloadChildModule', 'didUnloadChildModule', 'onUnload', 'didUnload', 'onDispose' ] Actual: [ 'onWillLoadChildModule', 'willLoadChildModule', 'onShouldUnload', 'willUnload', 'onDidLoadChildModule', 'onWillUnloadChildModule', 'didLoadChildModule', 'willUnloadChildModule', 'onDidUnloadChildModule', 'didUnloadChildModule', 'onUnload', 'didUnload', 'onDispose' ] Which: was 'onWillUnloadChildModule' instead of 'didLoadChildModule' at location [5] package:test expect test/lifecycle_module_test.dart 2236:11 runTests.<fn>.<fn>.<fn>.<fn>

evanweible-wf · 2018-11-08T17:15:16Z

QA +1

CI passes on Dart 1 and Dart 2

@Workiva/release-management-p

initial changes for dart 2

1422f6e

robbecker-wf requested a review from a team as a code owner October 24, 2018 03:28

robbecker-wf requested a review from a team October 24, 2018 03:28

rmconsole-wf added the Merge Requirements Unmet label Oct 24, 2018

robrbecker added 4 commits October 23, 2018 21:32

only run coverage under dart1

8558d98

use the dart 2 compat published w_flux

77e83a6

fix tests

a48e41c

- adjust a type error - work around sync-async issues - shorten dockerfile run since everything happens in Travis CI

add working directory to dockerfile, format check under dart2 only

966f93b

robbecker-wf commented Oct 27, 2018

View reviewed changes

fix types in examples

502dc3f

robbecker-wf changed the title ~~WIP dart 2~~ Dart 2 compatibility Oct 28, 2018

rmconsole6-wk changed the title ~~Dart 2 compatibility~~ AF-2971 WIP dart 2 Oct 28, 2018

robbecker-wf changed the title ~~AF-2971 WIP dart 2~~ AF-2971 Dart 2 compatibility Oct 28, 2018

robbecker-wf commented Oct 28, 2018

View reviewed changes

Dockerfile

@@ -1,43 +1,6 @@

FROM google/dart:1.24.3 as build

Copy link

Member Author

robbecker-wf Oct 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just let Travis CI run all the tests.

remove extra await that doesn't cause tests to fail

df75053

robbecker-wf commented Oct 29, 2018

View reviewed changes

sebastianmalysa-wf approved these changes Nov 7, 2018

View reviewed changes

evanweible-wf approved these changes Nov 8, 2018

View reviewed changes

rmconsole-wf added RM Ready Merge Requirements Met and removed Merge Requirements Unmet labels Nov 8, 2018

rmconsole5-wk merged commit 714dfaa into Workiva:master Nov 8, 2018

robbecker-wf deleted the dart2 branch November 8, 2018 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

AF-2971 Dart 2 compatibility #137

AF-2971 Dart 2 compatibility #137

Uh oh!

robbecker-wf commented Oct 24, 2018 •

edited

Loading

Uh oh!

aviary3-wk commented Oct 24, 2018

Uh oh!

robbecker-wf Oct 27, 2018

Uh oh!

robbecker-wf Oct 27, 2018

Uh oh!

robbecker-wf Oct 31, 2018

Uh oh!

robbecker-wf Oct 27, 2018

Uh oh!

robbecker-wf Oct 31, 2018

Uh oh!

robbecker-wf Oct 27, 2018

Uh oh!

robbecker-wf Oct 29, 2018

Uh oh!

codecov-io commented Oct 28, 2018

Uh oh!

robbecker-wf Oct 28, 2018

Uh oh!

robbecker-wf Oct 29, 2018

Uh oh!

robbecker-wf Nov 8, 2018

Uh oh!

evanweible-wf commented Nov 8, 2018

Uh oh!

Uh oh!

AF-2971 Dart 2 compatibility #137

AF-2971 Dart 2 compatibility #137

Uh oh!

Conversation

robbecker-wf commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Changes

Testing

Uh oh!

aviary3-wk commented Oct 24, 2018

Security Insights

Action Items

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 Oct 28, 2018

Codecov Report

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!

evanweible-wf commented Nov 8, 2018

Uh oh!

Uh oh!

robbecker-wf commented Oct 24, 2018 •

edited

Loading