Skip to content

test: add tests for NotificationIcon and RepositorySectionTitle#677

Merged
housseindjirdeh merged 1 commit intogitpoint:masterfrom
ZahraTee:tests-repositorySectionTitle-notificationIcon
Dec 22, 2017
Merged

test: add tests for NotificationIcon and RepositorySectionTitle#677
housseindjirdeh merged 1 commit intogitpoint:masterfrom
ZahraTee:tests-repositorySectionTitle-notificationIcon

Conversation

@ZahraTee
Copy link
Copy Markdown
Contributor

@ZahraTee ZahraTee commented Dec 20, 2017

Given NotificationIcon is a connected component, I decided just to export and test the undecorated component inside, I can look into testing the connected component too if necessary. There aren't any other examples in the repo yet to indicate what to do in this case. :)

(Part of #518)

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.6%) to 40.469% when pulling 3aa67e5 on ZahraTee:tests-repositorySectionTitle-notificationIcon into 4e5235e on gitpoint:master.

Copy link
Copy Markdown
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

Looks really good, thanks so much!! Just one question:

});

class NotificationIconComponent extends Component {
export class NotificationIconComponent extends Component {
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.

Why does this need to be exported? Can you test NotificationIcon which is exported at the bottom of this file?

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.

Given NotificationIcon is this component but connected to a Redux store, the options were to either just test this inner component or to test NotificationIcon itself and mock out the store etc. I can look into the latter option.

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.

Although exporting and testing the unwrapped Component seems to be recommended by Redux: https://redux.js.org/docs/recipes/WritingTests.html#connected-components

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.

Yep, you're right! This should be exported.

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.

(not to self: we might want to make this a dumb component w/o access to redux state and do all that state management elsewhere)

Copy link
Copy Markdown
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

This is so great, thank you so much @ZahraTee :)

"contributions": [
"code",
"test"
]
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.

🎉

@housseindjirdeh housseindjirdeh merged commit b0e8bf0 into gitpoint:master Dec 22, 2017
@andrewda andrewda mentioned this pull request Dec 1, 2018
63 tasks
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.

5 participants