FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(unit tests): tests now pass in util.test.js by jenny-s51 · Pull Request #2489 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

fix(unit tests): tests now pass in util.test.js - #2489

Merged
redallen merged 5 commits into
patternfly:masterfrom
jenny-s51:iss1624
Jul 9, 2019
ConversationCommits (5)ChecksFiles changed
Merged

fix(unit tests): tests now pass in util.test.js#2489
redallen merged 5 commits into
patternfly:masterfrom
jenny-s51:iss1624

Conversation

jenny-s51 commented Jul 9, 2019

Copy link
Copy Markdown
Contributor

What: Closes #1624 and fixes keyHandler tests in util.test.js -- tests are now passing.

patternfly-build commented Jul 9, 2019

Copy link
Copy Markdown
Collaborator

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

jeff-phillips-18 reviewed Jul 9, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/src/helpers/util.test.js Outdated
secondDropdownItem.simulate('keydown', { key: 'ArrowUp', keyCode: KEY_CODES.ARROW_UP, which: KEY_CODES.ARROW_UP });
expect(firstDropdownItem === document.activeElement).toBe(true);
let focusedElement = document.activeElement;
expect(focusedElement.getAttribute('id')).toEqual('second');

jeff-phillips-18 Jul 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would remove the initial expected focus item check from each of the tests. This makes each test depend on the previous test.

TheRealJon Jul 9, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since there is no set-up or tear-down between each test case, they will depend on the previous test regardless. The following tests will fail if the DOM is not in the correct state. Maybe we should be initializing the DOM state from scratch on each test case?

jenny-s51 Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jeff-phillips-18 what do you think?

jeff-phillips-18 Jul 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it's ok to just remove the init checks. The initial state is not handled by the utils and thus need not be tested here.

TheRealJon reviewed Jul 9, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/src/helpers/util.test.js Outdated
Comment thread packages/patternfly-4/react-core/src/helpers/util.test.js Outdated
Comment thread packages/patternfly-4/react-core/src/helpers/util.test.js Outdated
secondDropdownItem.simulate('keydown', { key: 'ArrowUp', keyCode: KEY_CODES.ARROW_UP, which: KEY_CODES.ARROW_UP });
expect(firstDropdownItem === document.activeElement).toBe(true);
let focusedElement = document.activeElement;
expect(focusedElement.getAttribute('id')).toEqual('second');

TheRealJon Jul 9, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since there is no set-up or tear-down between each test case, they will depend on the previous test regardless. The following tests will fail if the DOM is not in the correct state. Maybe we should be initializing the DOM state from scratch on each test case?

jenny-s51 and others added 2 commits July 9, 2019 14:03
remove eslint-disable-next-line
bb58fc0 
Co-Authored-By: Jon Jackson <jonjacks@redhat.com>
remove eslint-disable-next-line
560a389 
Co-Authored-By: Jon Jackson <jonjacks@redhat.com>
jeff-phillips-18 approved these changes Jul 9, 2019
View reviewed changes

jeff-phillips-18 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM! 👍

redallen approved these changes Jul 9, 2019
View reviewed changes

redallen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Jest is fun :) 👍

redallen merged commit 86981f6 into patternfly:master Jul 9, 2019
jenny-s51 deleted the iss1624 branch July 17, 2019 18:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jeff-phillips-18 jeff-phillips-18 approved these changes
TheRealJon Awaiting requested review from TheRealJon
+1 more reviewer
redallen redallen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unit tests for util.ts are broken.

5 participants

Back | FazBrowse Home | New Git URL