Skip to content

Conversation

@oandregal
Copy link
Member

This PR prepares our test suite to be testable against the stable Context API, so we can migrate our code safely. Once this lands, we can merge #11123

@oandregal oandregal self-assigned this Oct 31, 2018
@oandregal oandregal added the Framework Issues related to broader framework topics, especially as it relates to javascript label Oct 31, 2018
@oandregal
Copy link
Member Author

As per @gziolo suggestion here I've tried not to use snapshot testing, but having the HTML inside the test. It looks like it's more difficult than it's supposed to and I couldn't figure out.

I can give it a second try if that's a blocker. I'm actually inclined to think that snapshot testing is a fine choice here.

@oandregal oandregal requested a review from a team October 31, 2018 16:14
@oandregal oandregal added this to the WordPress 5.0 milestone Oct 31, 2018
@gziolo gziolo requested review from aduth and nerrad October 31, 2018 17:00
@gziolo
Copy link
Member

gziolo commented Oct 31, 2018

@aduth @nerrad as you have some experience with writing test using react-test-renderer. Is there an easy way to replicate the following assertion written with enzyme:

expect( element.html() ).toBe( '<div><span></span></div>' );

If not let's use snapshots instead. My only concern is that having 10 snapshots for so simple HTML snippets makes those tests much harder to read. However, it doesn't seem to be a blocker here.

@gziolo gziolo added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Oct 31, 2018
@nerrad
Copy link
Contributor

nerrad commented Oct 31, 2018

If you're just testing a simple structure like that (i.e. no expected css class or props) I would think snapshots are fine. Provided the test has good enough description the snapshots aren't too hard to follow. As soon as you get to testing unit written components though you have to keep in mind that toJson doesn't include them.

@oandregal oandregal force-pushed the update/tests-for-context-api branch from 6ac3037 to 4e5b61c Compare November 2, 2018 10:40
@youknowriad youknowriad modified the milestones: WordPress 5.0, 4.3 Nov 2, 2018
@oandregal
Copy link
Member Author

Rebased from master.

@youknowriad
Copy link
Contributor

@gziolo

expect( element.html() ).toBe( '<div><span></span></div>' );

Why can just do :)

const div = document.createElement( 'div' );
render( something, div );
console.log( div.innerHTML );

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Tests pass, that's good enough for me 👍

I don't want to block the other PR

@oandregal oandregal merged commit 94d2b7f into master Nov 2, 2018
@oandregal
Copy link
Member Author

Cool, going to merge both PRs. We can always iterate on how tests are written.

@aduth aduth deleted the update/tests-for-context-api branch November 2, 2018 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Framework Issues related to broader framework topics, especially as it relates to javascript [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.

5 participants