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

Async support for TabPanel.canTabChange #432

Merged
merged 8 commits into from
May 15, 2020
Merged

Conversation

dperez3
Copy link
Contributor

@dperez3 dperez3 commented May 14, 2020

No description provided.

@dperez3
Copy link
Contributor Author

dperez3 commented May 14, 2020

Although the tests, pass there is an annoying error logged. Initial research indicates that newer versions of react and/or react testing library have fixed this, but it may also be do to non-ideal handling of the callback in the component. Maybe useEffect is warranted here though it seems like useState should be ok to invoke from an event handler according to documentation.

PASS src/components/containers/tabPanels/TabPanel.specs.js
  ● Console
    console.error node_modules/react-dom/cjs/react-dom.development.js:506
      Warning: An update to TabPanel inside a test was not wrapped in act(...).
      
      When testing, code that causes React state updates should be wrapped into act(...):
      
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */
      
      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
          in TabPanel
          in ThemeProvider

https://travis-ci.com/github/WTW-IM/es-components/jobs/333542674#L283-L298

Looking to fix this next...

Copy link
Collaborator

@Darrken Darrken left a comment

Choose a reason for hiding this comment

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

Looks good! I like how this ended up being a pretty simple small change.

@dperez3 dperez3 marked this pull request as ready for review May 14, 2020 21:53
@dperez3
Copy link
Contributor Author

dperez3 commented May 14, 2020

Thanks for the Approval fellas (@Darrken & @stevematney). I couldn't help but try and spend some time in figuring out the new act() messages showing up in test output and made a few more updates. It wasn't enough unfortunately 😞, but I left some notes for the next person who'd like to tackle it.

Part of me feels like something is lurking based on my research (see comment below), but it must be minor because the tests and the style guide do indicate that it's ultimately working.
testing-library/react-testing-library#535 (comment)

@dperez3
Copy link
Contributor Author

dperez3 commented May 14, 2020

Whoa, just found a UI issue. When resolving the Promise with false, the hover styling stays on the clicked Tab until another part of the page is clicked. Looking into fixing that now...

@dperez3
Copy link
Contributor Author

dperez3 commented May 14, 2020

Whoa, just found a UI issue. When resolving the Promise with false, the hover styling stays on the clicked Tab until another part of the page is clicked. Looking into fixing that now...

Actually, this looks to be an existing issue. I'll go ahead and post it, #433

Comment on lines 9 to 14
/*
The addition of async support has introduced a strange log message in the tests...
"When testing, code that causes React state updates should be wrapped into act(...):"
I was unable to resolve this, but this is where my research led me...
https://github.com/testing-library/react-testing-library/issues/535#issuecomment-576816390
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should be able to resolve this by wrapping things like the click in line 38 in an act call. https://reactjs.org/docs/test-utils.html#act

Copy link
Contributor Author

@dperez3 dperez3 May 14, 2020

Choose a reason for hiding this comment

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

I've tried different variations of it and none of them seem to work..

  • import { cleanup, wait, act } from 'react-testing-library'
  • import { act } from "react-dom/test-utils"
  • act(() => { ... });
  • await act(async () => { ... });

Copy link
Contributor Author

@dperez3 dperez3 May 14, 2020

Choose a reason for hiding this comment

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

Well, I'll be - @testing-library/react (formerly react-testing-library) addresses this exact issue...

There is a known compatibility issue with React DOM 16.8 where you will see the
following warning:

Warning: An update to ComponentName inside a test was not wrapped in act(...).

If you cannot upgrade to React DOM 16.9, you may suppress the warnings by adding
the following snippet to your test configuration
(learn more):

// this is just a little hack to silence a warning that we'll get until we
// upgrade to 16.9: https://github.com/facebook/react/pull/14853
const originalError = console.error
beforeAll(() => {
  console.error = (...args) => {
    if (/Warning.*not wrapped in act/.test(args[0])) {
      return
    }
    originalError.call(console, ...args)
  }
})

afterAll(() => {
  console.error = originalError
})

https://github.com/testing-library/react-testing-library/blob/master/README.md#suppressing-unnecessary-warnings-on-react-dom-168

This tests workaround looks to be working. Pushing shortly...

Also...
react-testing-library is now @testing-library/react. Project is using [email protected] whereas @testing-library/react is at 10.0.4. An update may be in order soon and will require updates to multiple test files from my quick attempt at it

@Darrken Darrken merged commit 1d77f8f into master May 15, 2020
@Darrken Darrken deleted the tab-panel-canTabChange-async branch May 15, 2020 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants