UIP-2512 Add more utils from over_react #11

aaronlademann-wf · 2017-07-13T16:34:50Z

Ultimate problem:

There were some test utilities within over_react that were not exposed / available for consumers.

How it was fixed:

Move them here, and expose them!

Testing suggestions:

Passing CI

Areas of regression

None expected

FYA: @jacehensley-wf @greglittlefield-wf @clairesarsam-wf

+ Unless we’re prepared to address them, I’d prefer that we turn them off.

+ We don’t have nearly enough tests in this lib to need to run them all syncronously.

+ For things like prop forwarding

aviary2-wf · 2017-07-13T16:34:58Z

Raven

Number of Findings: 0

codecov-io · 2017-07-13T16:41:42Z

Codecov Report

Merging #11 into master will decrease coverage by 43.4%.
The diff coverage is 4.79%.

@@             Coverage Diff             @@
##           master      #11       +/-   ##
===========================================
- Coverage   97.62%   54.23%   -43.39%     
===========================================
  Files           5        9        +4     
  Lines         210      402      +192     
===========================================
+ Hits          205      218       +13     
- Misses          5      184      +179

greglittlefield-wf · 2017-07-13T16:42:27Z

test/over_react_test/validation_util_test.dart

+
+    group('startRecordingValidationWarnings()', () {
+      test('begins recording validation warnings, appending them to `_validationWarnings` as expected', () {
+        react_dom.render(TestValidationWarnings()(), mountNode);


Using a component to test this seems really unnecessary. Why not just call ValidationUtil.warn directly in this test?

greglittlefield-wf · 2017-07-13T16:45:16Z

lib/src/over_react_test/string_util.dart

+// limitations under the License.
+
+/// Utility function to convert a `SCREAMING_SNAKE` [str] into a `spinal-case` equivalent.
+String screamingSnakeToSpinal(String str) {


Is this one-liner really worth including?

String screamingSnakeToSpinal(String str) => str.replaceAll('_', '-').toLowerCase();

I'm not opposed to leaving it in, but it seems extraneous.

Maybe not (shrug)

Yeah, I purposefully omitted this when moving stuff over

greglittlefield-wf · 2017-07-13T16:47:14Z

analysis_options.yaml

    # - prefer_final_fields
    # - prefer_final_locals
-    - prefer_function_declarations_over_variables
+    # - prefer_function_declarations_over_variables


Why was this lint disabled? Seems like it'd be valuable to keep enabled.

To get rid of noisy lints that we have no intention of addressing.

If someone wants to address them, thats cool - but the analyzer view within the IDE is most useful when it is empty when you start your work.

greglittlefield-wf · 2017-07-13T16:49:10Z

lib/src/over_react_test/js_component.dart

 import 'package:react/react_client/js_interop_helpers.dart';

-/// A factory for a JS composite component.
+/// A factory for testing a JS composite component.


#nit This is a little confiusing. What about:

A factory for a JS composite component, for use in testing.

This reverts commit c6d225b.

jacehensley-wf

Two small comments

jacehensley-wf · 2017-07-14T23:43:29Z

lib/src/over_react_test/js_component.dart

@@ -13,12 +13,12 @@
 // limitations under the License.

 import 'package:js/js.dart';
-import 'package:react/react.dart' as react;
+import 'package:react/react.dart' as react show div;


Why was this changed?

not sure tbh @jacehensley-wf - is it a problem? I may have just done it to make it easier to grok what specifically is needed from react.dart in this file since in general - we want to minimize direct dependencies on react, and prefer depending on the exports provided in over_react.

No it doesn't matter, was just curious 😄

jacehensley-wf · 2017-07-14T23:43:35Z

lib/src/over_react_test/js_component.dart

 import 'package:react/react_client.dart';
-import 'package:react/react_client/react_interop.dart';
+import 'package:react/react_client/react_interop.dart' show React, ReactClassConfig;


Why was this changed?

not sure tbh @jacehensley-wf - is it a problem? I may have just done it to make it easier to grok what specifically is needed from react.dart in this file since in general - we want to minimize direct dependencies on react, and prefer depending on the exports provided in over_react.

jacehensley-wf · 2017-07-19T17:07:25Z

+1

leviwith-wf · 2017-07-24T15:52:31Z

QA +10

build passes
tests also pass locally

Merging.

+ Looks like some changes had been made in parallel in the lib where these originated - and those changes never made it into #11

aaronlademann-wf added 10 commits July 13, 2017 09:18

Relax lints

493f731

+ Unless we’re prepared to address them, I’d prefer that we turn them off.

Address outstanding lints

877ca00

Bump test concurrency

81c0d81

+ We don’t have nearly enough tests in this lib to need to run them all syncronously.

Expose over_react.ValidationUtil test utils

f87edea

Expose common component tests

7eb98cb

+ For things like prop forwarding

Add string utils

c6d225b

Add zone utils

3b3841b

Expose js component util

902be17

Fix typo

04c2452

Add link to API docs

0bda78e

greglittlefield-wf reviewed Jul 13, 2017

View reviewed changes

This was referenced Jul 13, 2017

UIP-2463 Release OverReact 1.14.1 Workiva/over_react#96

Closed

UIP-2272 Remove test utilities that belong in over_react_test Workiva/over_react#95

Merged

aaronlademann-wf added 4 commits July 13, 2017 10:25

Remove unnecessary component for tests

8caf0dc

Revert "Add string utils"

3c66e21

This reverts commit c6d225b.

Improve doc comment

5df6ce3

Add tests for commonComponentTests

59bb9c2

jacehensley-wf reviewed Jul 14, 2017

View reviewed changes

aaronlademann-wf changed the title ~~Add more utils from over_react~~ UIP-2512 Add more utils from over_react Jul 18, 2017

rmconsole-wf added the Merge Requirements Met label Jul 19, 2017

leviwith-wf merged commit f93906c into Workiva:master Jul 24, 2017

This was referenced Jul 24, 2017

UIP-2465 Release over_react_test 1.1.0 #12

Merged

Consume over_react_test 1.1.0 Workiva/over_react#100

Closed

aaronlademann-wf added a commit that referenced this pull request Jul 26, 2017

Fix common component tests

d0fd23b

+ Looks like some changes had been made in parallel in the lib where these originated - and those changes never made it into #11

aaronlademann-wf mentioned this pull request Jul 27, 2017

UIP-2551 Release over_react_test 1.1.1 #14

Merged

aaronlademann-wf mentioned this pull request Aug 3, 2017

UIP-2546 Release OverReact 1.15.1 Workiva/over_react#102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIP-2512 Add more utils from over_react #11

UIP-2512 Add more utils from over_react #11

aaronlademann-wf commented Jul 13, 2017 •

edited

Loading

aviary2-wf commented Jul 13, 2017 •

edited

Loading

codecov-io commented Jul 13, 2017 •

edited

Loading

greglittlefield-wf Jul 13, 2017

greglittlefield-wf Jul 13, 2017

aaronlademann-wf Jul 13, 2017

jacehensley-wf Jul 13, 2017

greglittlefield-wf Jul 13, 2017 •

edited

Loading

aaronlademann-wf Jul 13, 2017

greglittlefield-wf Jul 13, 2017

jacehensley-wf left a comment

jacehensley-wf Jul 14, 2017

aaronlademann-wf Jul 17, 2017

jacehensley-wf Jul 17, 2017

jacehensley-wf Jul 14, 2017

aaronlademann-wf Jul 17, 2017

jacehensley-wf commented Jul 19, 2017

leviwith-wf commented Jul 24, 2017

UIP-2512 Add more utils from over_react #11

UIP-2512 Add more utils from over_react #11

Conversation

aaronlademann-wf commented Jul 13, 2017 • edited Loading

Ultimate problem:

How it was fixed:

Testing suggestions:

Areas of regression

aviary2-wf commented Jul 13, 2017 • edited Loading

Raven

codecov-io commented Jul 13, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf commented Jul 19, 2017

leviwith-wf commented Jul 24, 2017

aaronlademann-wf commented Jul 13, 2017 •

edited

Loading

aviary2-wf commented Jul 13, 2017 •

edited

Loading

codecov-io commented Jul 13, 2017 •

edited

Loading

greglittlefield-wf Jul 13, 2017 •

edited

Loading