Skip to content

Conversation

@abalakh
Copy link
Contributor

@abalakh abalakh commented Aug 14, 2019

What: Added OUIA props to Tabs as per #2425

Additional issues: 2 Tabs tests are failing after this change. Any help with figuring out what's causing the issue is appreciated.

@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://patternfly-react-pr-2704.surge.sh

@quarckster
Copy link
Contributor

@redallen redallen self-assigned this Aug 15, 2019
redallen
redallen previously approved these changes Aug 15, 2019
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Fixed snapshots :)

key="2/.2"
/>
</Fragment>
<ContextConsumer>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@redallen first off, thanks for help. But are you sure this snap will actually cover test scenario? It's basically empty, and it doesn't look like it will ensure "should render tabs with eventKey Strings".
Appologies if it's a dumb question, i'm pretty new to jest testing, trying to understand to not introduce same mistakes in future PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff is too big so GH points to wrong line. My question was re this snap:

exports[`should render tabs with eventKey Strings 1`] = `
<ContextConsumer>
  <Component />
</ContextConsumer>
`;

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it makes sense to make the snapshot tests that use OUIA use mount instead of shallow since there's now a <ContextConsumer>.

@abalakh
Copy link
Contributor Author

abalakh commented Aug 16, 2019

@redallen so i returned branch to previous state with mount instead of shallow. The thing is, 2 tests are failing with mount (snaps for them aren't updated as they can't be generated yet), see ci/circleci: test_jest_pf4 result. Any ideas what's causing failures?

@redallen
Copy link
Contributor

Yes, this is the same issue I fixed yesterday. You have to fix the selectors Jest uses for those tests since now there's a <ContextConsumer><Component /></ContextConsumer> in the VDOM.

@abalakh
Copy link
Contributor Author

abalakh commented Aug 19, 2019

@redallen not sure if i follow. Those 2 failing tests are comparing mount result with snap, no interactions and no css selectors are used there.

@karelhala
Copy link
Contributor

Thanks for adding this! I think that the best way would be to replace shallow test renders with render from enzyme. We don't really test ouia so no need to check if that is present in snapshots and mount just adds a lot of code to snaps that we really don't care right? Plus render is a bit faster than mount (doesn't have to call componentDidMount etc.).

@redallen
Copy link
Contributor

Good call @karelhala , I've updated the PR with that, and I wouldn't have thought of that 👍

I am rather confused why mount was having problems only for these two tests:

test('should render secondary tabs', () => {
  const view = render(
    <Tabs id="primarieTabs">
      <Tab eventKey={0} title="Tab item 1">
        <Tabs id="secondaryTabs">
          <Tab id="secondary tab1" eventKey={10} title="Secondary Tab 1">
            Secondary Tab 1 section
          </Tab>
          <Tab id="secondary tab2" eventKey={11} title="Secondary Tab 2">
            Secondary Tab 2 section
          </Tab>
          <Tab id="secondary tab3" eventKey={12} title="Secondary Tab 3">
            Secondary Tab 3 section
          </Tab>
        </Tabs>
      </Tab>
      <Tab id="tab2" eventKey={1} title="Tab item 2">
        Tab 2 section
      </Tab>
      <Tab id="tab3" eventKey={2} title="Tab item 3">
        Tab 3 section
      </Tab>
    </Tabs>
  );
  expect(view).toMatchSnapshot();
});
test('should render tabs with eventKey Strings', () => {
  const view = render(
    <Tabs id="eventKeyTabs">
      <Tab id="tab1" eventKey={'one'} title="Tab item 1">
        Tab 1 section
      </Tab>
      <Tab id="tab2" eventKey={'two'} title="Tab item 2">
        Tab 2 section
      </Tab>
      <Tab id="tab3" eventKey={'three'} title="Tab item 3">
        Tab 3 section
      </Tab>
    </Tabs>
  );
  expect(view).toMatchSnapshot();
});

Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

Looking good!

@jschuler jschuler merged commit e759244 into patternfly:master Aug 23, 2019
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • @patternfly/react-charts@4.8.3
  • @patternfly/react-core@3.91.0
  • @patternfly/react-docs@4.10.24
  • @patternfly/react-inline-edit-extension@2.10.19
  • demo-app-ts@2.21.3
  • @patternfly/react-table@2.18.13
  • @patternfly/react-topology@2.7.44
  • @patternfly/react-virtualized-extension@1.1.130
  • @patternfly/react-icons@3.12.0

Thanks for your contribution! 🎉

@abalakh abalakh deleted the pf4_ouia_tabs branch August 23, 2019 17:41
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.

6 participants