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

Add tests for Editable.adaptFormatter() #3894

Merged
merged 1 commit into from
Dec 23, 2017

Conversation

BE-Webdesign
Copy link
Contributor

Add tests for Editable.adaptFormatter()

@BE-Webdesign BE-Webdesign added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Dec 9, 2017
@gziolo
Copy link
Member

gziolo commented Dec 11, 2017

I have one concern here. This test covers private function from the component. There was usually a rule to test only public API in the previous projects, I worked with. Do you think we should keep the same rule in Gutenberg? We can always move that method to the file with utilities when we fill that it deserves its own tests.

cc @aduth @youknowriad @mtias

@gziolo
Copy link
Member

gziolo commented Dec 11, 2017

If we are fine with testing private methods, then it's good to go 👍

@BE-Webdesign
Copy link
Contributor Author

One option is if these private methods are used somewhere in a public facing code we can get coverage that way. For now going to leave the PR open.

test( 'should return an object on inline: span, and a styles property matching the style object provided', () => {
expect( wrapper.instance().adaptFormatter( options ) ).toEqual( {
inline: 'span',
styles: { ...options.style },
Copy link
Member

Choose a reason for hiding this comment

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

It can also simply styles: options.style,. No need to clone it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes I do silly things 🙃

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's merge it, we can change our approach about testing private methods later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants