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

chore(tests): fail on console activity #1542

Merged
merged 47 commits into from
Jun 1, 2018
Merged

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Apr 3, 2017

Fixes #1507

This PR overloads console methods to throw. This fails all tests that result in console activity. This reveals several issues with props, keys, and other bad usages.

Karma now also serves the doc files. This makes 404s in tests relevant as it means the doc site itself is missing these assets as well. I'd like to throw on 404 if possible. This way, we also know all doc site assets exist.

TODO

@layershifter
Copy link
Member

❤️

@layershifter
Copy link
Member

#1599 was merged, unblocking.

@codecov-io
Copy link

codecov-io commented Aug 30, 2017

Codecov Report

Merging #1542 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1542      +/-   ##
==========================================
+ Coverage   99.66%   99.66%   +<.01%     
==========================================
  Files         161      161              
  Lines        2714     2722       +8     
==========================================
+ Hits         2705     2713       +8     
  Misses          9        9
Impacted Files Coverage Δ
src/behaviors/Visibility/Visibility.js 100% <100%> (ø) ⬆️
src/addons/Select/Select.js 100% <100%> (ø) ⬆️
src/modules/Embed/Embed.js 100% <100%> (ø) ⬆️
src/collections/Form/FormSelect.js 100% <100%> (ø) ⬆️
src/modules/Popup/Popup.js 100% <100%> (ø) ⬆️
src/modules/Progress/Progress.js 100% <100%> (ø) ⬆️

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 500df9a...88171ee. Read the comment docs.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

@levithomason I've merged with master and fixed some tests, really want to see it finished 👍

@levithomason
Copy link
Member Author

levithomason commented Sep 30, 2017

Only 2 components left 😅

Portal
  ✖ "after each" hook for "does not call this.setState() if portal is unmounted"
    Error: console.error should never be called but was called with:
    Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the WrapperComponent component.

Table
  shorthand
    ✖ renders the table
      Error: console.error should never be called but was called with:
      Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `Table`. See https://fb.me/react-warning-keys for more information.

Table (Factories breaking change)

Seems we need a breaking change to factories here. Arrays cannot be used as shorthand values after all. We use literal values as keys. In the case of an array, there is no value to use as the key.

Table row shorthand allows passing an array as a shorthand value. It is mapped to cells. However, this then creates a prop object { cells: [] } with no key.

Portal

There is something setting state after unmount still. Seems we may need the setSafeState approach that was taken in Transition. Before adding that update, I'd like to know why state is being called after mount in the first place.

Note, Popup is failing as it uses the Portal.

@layershifter
Copy link
Member

Unbeviable, tests are passed ✌️

@layershifter
Copy link
Member

@levithomason I've reverted some style changes, marked as ready for review 👍

…React into fix/test-warnings

# Conflicts:
#	src/modules/Sticky/Sticky.js
@levithomason
Copy link
Member Author

Updating... hang tight

@levithomason
Copy link
Member Author

Over a year running, but we're actually going to merge this one 😅

@layershifter layershifter merged commit 3a30105 into master Jun 1, 2018
@layershifter layershifter deleted the fix/test-warnings branch June 1, 2018 10:42
@layershifter
Copy link
Member

Fixed last merge conflicts and merged 👍

@levithomason
Copy link
Member Author

Released in semantic-ui-react@0.81.0.

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.

chore(tests): fail on console.error for propType warnings
3 participants