Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[PLT-7814] Upgrade to enzyme-adapter-react-16 and enable unit/components tests at Jenkins build #303

Closed
wants to merge 3 commits into from

Conversation

saturninoabril
Copy link
Member

@saturninoabril saturninoabril commented Nov 14, 2017

Summary

This is the final batch for PLT-7814, resolving existing (and additional) component test failures.

Included in this PR:
(UPDATE) 1. Upgrade to enzyme-adapter-react-16 with the recent upgrade to React 16
- encountered several test suites failures like Error: Enzyme Internal Error: unknown node with tag 4 due to issue on Enzyme 3
- it turned out that Adapter React 16 does not support new Portal node types. Though we are not using Portals, our dependencies do like the Modal of react-bootstrap.
- affected test cases were temporarily disabled removed (instead of making workaround) until this PR enzymejs/enzyme#1263 to enzyme-adapter-react-16 gets merged.

  1. Add make test command
  2. Enable unit/components tests at Jenkins build.
  3. Add make test to PR checklist
  4. Add reducers and selectors folders for test coverage computation
  5. (UPDATE) Add "clearMocks": true to jest config to automatically clear mock calls and instances between every test.

Ticket Link

Jira ticket: PLT-7814

(UPDATE): Jira ticket to revert component tests - PLT-8133

Checklist

  • Ran make check-style to check for style errors (required for all pull requests)
  • Added or updated unit tests (required for all new features)
  • Touches critical sections of the codebase (Jenkins build, make file)

…with dependencies using react 16 Portals), enable jest at Jenkins build
@saturninoabril saturninoabril added the 2: Dev Review Requires review by a core commiter label Nov 14, 2017
@saturninoabril saturninoabril added this to the v4.5.0 milestone Nov 14, 2017
@ccbrown
Copy link
Contributor

ccbrown commented Nov 14, 2017

As a general principle, I'm really against commenting out tests since they have a tendency to just sit in the codebase and rot. If you intend to update these, I would recommend deleting them instead and giving yourself a JIRA ticket. That eliminates the possibility that they'll just rot and makes it much more likely that they'll get updated eventually, keeping our code prettier and more navigable in the meantime. It also allows you to better capture the work you have to do when planning or reviewing your sprints. Git was literally made for preserving old versions of our code. We don't need comments for that.

Copy link
Contributor

@ccbrown ccbrown left a comment

Choose a reason for hiding this comment

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

See comment. I feel pretty strongly about it, but I'm not gonna block the review on it.

@saturninoabril
Copy link
Member Author

I understand. I left it that way since most of those were written by community contributors. But anyway, I'll send update removing those commented lines, and will just put back whenever enzyme issue gets resolve.

@saturninoabril
Copy link
Member Author

Updated PR and description above, adding Jira ticket assigned to me - https://mattermost.atlassian.net/browse/PLT-8133 - to revert component tests.

@saturninoabril
Copy link
Member Author

I forgot this one. I added clearMocks (set to true) to jest config. This is to automatically clear mock calls and instances between every test.

@saturninoabril
Copy link
Member Author

I noticed that the awaited PR for enzyme-adapter-react-16 is now merged and so I upgraded this PR to [email protected], and then reverted all those previously disabled tests.

@crspeller
Copy link
Member

@saturninoabril Can you resubmit this as a branch on the mattermost-webapp repository? I have jenkins configured to not trust Jenkinsfile changes that are not from branches on the main repo.

@saturninoabril
Copy link
Member Author

@crspeller Noted. Submitted branch - #310

@saturninoabril saturninoabril deleted the PLT-7814-17 branch November 17, 2017 04:39
@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label Nov 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2: Dev Review Requires review by a core commiter Tests/Not Needed Does not require new release tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants