Usability issues

[ivan-kleshnin]

First I want to admit I like the idea behind this library. This is what I hoped to find. Saying that I discovered a number of usability issues.

Let's pretend I want to test stateless component like Pagination.

The most important thing to test against is probably the node tree and the library handles it good. Though the perfect way would be a visual diff like the one Mocha + Chai provide.

let expected = `
<nav>
  <ul>
    <li><a><span>«</span></a></li>
    <li><a><span>1</span></a></li>
    <li><a><span>2</span></a></li>
    <li><a><span>3</span></a></li>
    <li><a><span>»</span></a></li>
  </ul>
</nav>`;

reactDomAssertion.assertSameAsString(expected, rendered, {checkedAttributes: []}); 

First issue is that HTML entities. I was forced to replace » and « with their unicode counterparts. This is error prone. I surely can do HTML decoding of the input string but as it seems to be a constant step involved I would consider to move it to a library code.

Second, bigger issue. If I want to check for classes I must explicitly include (or don't exclude them). So I should either declare all of them or none.

I think the absence of class is very rare case to test and it can already be handled easily without this library by common react testing practices. So it should be better to treat expected string differently. As an enumeration of what is absolutely required to be found in a rendered node.

1. If we include attribute in expected string – we require its presence
2. If we don't include attribute – we're agnostic about it

Then we don't need to create and support that clunky white and black lists with magic "defaults". As I already said, If we still do need to query against some attributes in a more explicit form we always can fallback to lower level tools.

I believe this approach is more sane than the current one. You can also make a described one a default behavior with alternatives switched by flags but I tend to think that such global and rigid control is a bad style.

For some nodes you want to be more precise, for others – less precise and this is probably just impossible to preconfigure through options.

[avik-das]

Hi @ivan-kleshnin, thanks so much for the constructive feedback. I agree that the usability is not there yet, so I'm glad someone else can weigh in regarding what would be useful.

Though the perfect way would be a visual diff like the one Mocha + Chai provide.

Absolutely! I actually played around with this today, and it's tracked in #3. Hopefully I can publish a new version soon that at least works for Mocha (unfortunately, getting it to work with Jest might need some more work).

First issue is that HTML entities. I was forced to replace » and « with their unicode counterparts. This is error prone. I surely can do HTML decoding of the input string but as it seems to be a constant step involved I would consider to move it to a library code.

Tracked in #2.

1. If we include attribute in expected string – we require its presence
2. If we don't include attribute – we're agnostic about it

Interesting idea. I'll have to look at my uses of this library to see if this behavior works in my use cases. If so, great, I'll make the change. Otherwise, I'll see if there's a way to reconcile our goals. Intuitively, I like the suggestion, though!

[ivan-kleshnin]

Thank you!

1)

Absolutely! I actually played around with this today, and it's tracked in #3. Hopefully I can publish a new version soon that at least works for Mocha (unfortunately, getting it to work with Jest might need some more work).

I wonder if React diff tool could help with that. It probably has all things you need and even more because they already do complex DOM diffs with customizable constraints. Facebook team really had to expose it long ago.

Interesting idea. I'll have to look at my uses of this library to see if this behavior works in my use cases. If so, great, I'll make the change. Otherwise, I'll see if there's a way to reconcile our goals. Intuitively, I like the suggestion, though!

Yeah. I only shallowly scratched the surface with some limited inputs and my conclusions may really be off-board. Definitely need to be validated.

2)

The most important conclusion I think I've got is that to test React components we need to split render testing and event testing. Most people, including React devs mess this together in their public examples.

Why should we query DOM again just to compare HTML? This makes testing code very obsure. Same rules apply for every kind of code and tests should be readable and supportable as well as the main code IMO.

So, admitting I'm very green at React testing, I really believe we should split Component testing into 2 parts:

1) Give inputs (props, state) and validate output (render). Does not require DOM querying. 2) Simulate events and validate output (changed state). Requires DOM querying.

Your library covers first case and React TestUtils cover second case.

Something like that. And I'd like to know your opinion about this speculative conclusions.

[avik-das]

I wonder if React diff tool could help with that. It probably has all things you need and even more because they already do complex DOM diffs with customizable constraints.

Sorry, I should have been clearer. It already works great out of the box with Mocha if I just ensure both the expected and actual DOM trees are serialized in a consistent, deterministic manner:

But it uses a Mocha-only feature where the exception being thrown has to have a showDiff flag set, and the actual and expected strings. It then does a string-based diff. But, the question is how Jest (and really probably Jasmine) can be made to show the diff as well.

If I can't figure out the part about Jest, I'll still plan to release the Mocha-compatible version soon.

1. Give inputs (props, state) and validate output (render). Does not require DOM querying.
2. Simulate events and validate output (changed state). Requires DOM querying.

Barring the fact that we should aim to minimize state (thereby minimizing the need for the second type of test), this makes sense. And as you said, I'm going to focus on the first one with this library :)

[ivan-kleshnin]

Sorry, I should have been clearer. It already works great out of the box with Mocha if I just ensure both the expected and actual DOM trees are serialized in a consistent, deterministic manner:

Looks great :+1:

If I can't figure out the part about Jest, I'll still plan to release the Mocha-compatible version soon.

Just for a stat I use Mocha. I don't like vendor lock-ins like Jest.

[avik-das]

@ivan-kleshnin:

I published a new version of the module (0.5.3) with the visual diff functionality.

As per #2, I decided not to handle HTML entities specially just yet.

Finally, I think the attribute handling you suggest makes sense for my use cases too. I don't have the bandwidth now that the weekend is over, but I'll look into implementing it when I get some free cycles.

Thanks for the suggestions! I'm going to close this now, with #4 tracking the remaining work.