UIP-2512 Add more utils from over_react by aaronlademann-wf · Pull Request #11 · Workiva/over_react_test

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


 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.dart' as react show div;
 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

Conversation

aaronlademann-wf commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ultimate problem:

How it was fixed:

Testing suggestions:

Areas of regression

Uh oh!

aviary2-wf commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Raven

Uh oh!

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jacehensley-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!

jacehensley-wf commented Jul 19, 2017

Uh oh!

leviwith-wf commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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