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-2272 Remove test utilities that belong in over_react_test #95

Merged

Conversation

aaronlademann-wf
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf commented Jul 13, 2017

Depends on Workiva/over_react_test#11

Ultimate problem:

There were some test utilities in this library that were not exposed / available for consumers.

How it was fixed:

Move them to over_react_test

Testing suggestions:

Passing CI

Areas of regression

Tests


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

@aviary2-wf
Copy link

aviary2-wf commented Jul 13, 2017

Raven

Number of Findings: 0

tool/dev.dart Outdated
@@ -36,8 +36,7 @@ main(List<String> args) async {
// See https://github.com/Workiva/over_react/issues/36
// 'chrome',
]
// Prevent test load timeouts on Smithy.
..concurrency = 1
..concurrency = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit 4 is the default, this can just be removed

pubspec.yaml Outdated
dependency_overrides:
over_react_test:
git:
url: git@github.com:aaronlademann-wf/over_react_test.git
Copy link
Contributor

Choose a reason for hiding this comment

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

travis doesn't support SSH urls :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this will have to get removed before we merge anyway 🤷‍♂️

@aaronlademann-wf aaronlademann-wf changed the title Remove test utilities that belong in over_react_test UIP-2272 Remove test utilities that belong in over_react_test Jul 18, 2017
@rmconsole-wf
Copy link

@aaronlademann-wf This pull request has merge conflicts, please resolve.

+ And move all remaining utils out of the “wsd_test_utils” directory
+ Consumers of this lib have no idea what wsd is / means.
+ We don’t have enough tests in this lib to need the syncronous workaround for test timeouts.
@aaronlademann-wf aaronlademann-wf force-pushed the move-test-utils-to-over-react-test branch from 32476f5 to e9e989f Compare July 21, 2017 14:16
+ 4 is the default.
@aaronlademann-wf
Copy link
Contributor Author

@jacehensley-wf @greglittlefield-wf @clairesarsam-wf this no longer has a dependent PR, and is ready for a final pass.

Now that over_react_test 1.1.0 has been released, over_react analyzer will fail until this is merged / released.

@codecov-io
Copy link

codecov-io commented Jul 26, 2017

Codecov Report

Merging #95 into master will decrease coverage by 0.53%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   94.87%   94.34%   -0.52%     
==========================================
  Files          31       31              
  Lines        1537     1537              
==========================================
- Hits         1458     1450       -8     
- Misses         79       87       +8

@aaronlademann-wf
Copy link
Contributor Author

@jacehensley-wf @greglittlefield-wf @clairesarsam-wf this is ready for review.

@jacehensley-wf
Copy link
Contributor

+1

@leviwith-wf
Copy link
Contributor

QA +10

  • Build successful
  • tests pass

  • Testing instruction
  • Dev +1's
  • Unit tests created/updated
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

@leviwith-wf leviwith-wf merged commit b370c39 into Workiva:master Jul 28, 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