Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UIP-2430 Use over_react_test, rip out duplicated test utils #83

Merged

Conversation

greglittlefield-wf
Copy link
Contributor

Ultimate problem:

We had duplicated code for test utils that has since made its way into over_react_test.

How it was fixed:

Pull in over_react_test, rip out duplicated test utils

Testing suggestions:

  • Verify all tests pass in content-shell and chrome (should work on either Dart 1.23 or 1.24)
    • ddev test -p content-shell
    • ddev test -p chrome

Potential areas of regression:

Tests


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

@aviary-wf
Copy link

Raven

Number of Findings: 0

@@ -1101,3 +1101,12 @@ class PlainObjectPropsMap {
class PlainObjectStyleMap {
external get width;
}

/// Helper component that renders whatever you tell it to. Necessary for rendering components with the 'ref' prop.
final RenderingContainerComponentFactory =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was only needed by this file, so I moved it here from test/test_util/react_util.dart

@@ -35,7 +35,7 @@ main() {
test('root contains the other element', () {
var rootInstance = render(DomTest());
var rootNode = findDomNode(rootInstance);
var otherNode = getDomByTestId(rootInstance, 'innerComponent');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was deprecated

@@ -265,8 +265,6 @@ void testClassNameMerging(BuilderOnlyUiFactory factory, dynamic childrenFactory(
excludesClasses('blacklisted-class-1 blacklisted-class-2')
)
));

unmount(renderedInstance);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was being auto-unmounted, so this was unnecessary

@@ -278,8 +276,6 @@ void testClassNameMerging(BuilderOnlyUiFactory factory, dynamic childrenFactory(
var descendantsWithCustomClass = react_test_utils.scryRenderedDOMComponentsWithClass(renderedInstance, customClass);

expect(descendantsWithCustomClass, hasLength(1));

unmount(renderedInstance);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was being auto-unmounted, so this was unnecessary

@@ -289,7 +285,8 @@ void testClassNameOverrides(BuilderOnlyUiFactory factory, dynamic childrenFactor
var reactInstanceWithoutOverrides = render(
(factory()
..addProp(forwardedPropBeacon, true)
)(childrenFactory())
)(childrenFactory()),
autoTearDown: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was being unmounted manually later on, so this was necessary

export 'dom_util.dart';
export 'react_util.dart';
export 'wrapper_component.dart';
export 'package:over_react_test/over_react_test.dart';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just exported these instead of updating the imports in every file, in order to get this through more quickly and with less risk of merge conflicts.

@rmconsole2-wf rmconsole2-wf changed the title Use over_react_test, rip out duplicated test utils UIP-2430 Use over_react_test, rip out duplicated test utils Jun 21, 2017
@codecov-io
Copy link

Codecov Report

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

@@           Coverage Diff           @@
##           master      #83   +/-   ##
=======================================
  Coverage   94.81%   94.81%           
=======================================
  Files          31       31           
  Lines        1522     1522           
=======================================
  Hits         1443     1443           
  Misses         79       79

@jacehensley-wf
Copy link
Contributor

+10

  • Tests pass in content-shell
  • Tests pass in chrome

image

@clairesarsam-wf
Copy link
Contributor

QA +10

  • Testing instructions
    • Tests pass on content-shell running Dart 1.23.0
    • Tests pass on chrome running Dart 1.23.0
  • Dev +1
  • Dev/QA +10
  • Unit tests created/updated
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

@clairesarsam-wf clairesarsam-wf merged commit 98e6736 into Workiva:master Jun 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants