Divider: switch to snapshot tests

[kalinkrustev]

Proposal to switch to snapshot testing. The benefits are:

1. Test code is much simpler, focusing on running the different test cases
2. The rendered markup is checked in full, not just some classes
3. When there is unexpected change, it is easier to track in the snapshots

[github-actions]

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

[kalinkrustev]

The original test is more complicated, than it need to be. Did you notice you have the same test multiple times?

[bahadirsofuoglu]

I noticed this. But here I see the benefit of the function you wrote more than the snapshot benefit. You have created html code for all cases. The only thing we want to check is the existence of a class in render. If we created the same function for the old structure, which one would you think is advantageous? Please let me know if there's anything I missed. I will be happy to learn.

I am talking about this function:
const snapshot = (props, name) => expect(render(<Divider {...props} />).container).toMatchSnapshot(name);

[melloware]

So the real advantage of Snapshot testing over this current test. lets take for example this:

expect(container.getElementsByClassName('p-divider p-component p-divider-horizontal p-divider-center').length).toBe(1);

This test will NOT fail if the following is changed:

1. If Divider is changed from a DIV to a SPAN this test would still pass and it should not the structure of the component has changed.
2. If a new style class p-new-class was introduced in the component this test would still pass because the current query will still be true. And to me this test should fail if a NEW class is added to the component or an unexpected one is there like p-filled when it should not be etc.

So the benefits to me are tremendous.

[kalinkrustev]

Did you notice you repeated the same test message? It is good for the test message to reflect the case you are testing.

[kalinkrustev]

Currently Code Format check fails on master, that's why it also failed here.

[melloware]

I fixed it if you want to merge master

[melloware]

I think we are also missing tests where there is left , right, and center text on the Divider?

[melloware]

Oh no i see you have those!

[github-actions]

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

[melloware]

This is great especially for components like Divider, Badge, Chip etc. But when you need events like my InputText and InputTextArea tests do you still Snapshot those or do you write them like I did with events? Or do you mix and match?

[kalinkrustev]

You can mix and match. Snapshots substitute the need to check individual markup details manually. For example there is no need to maintain this verbose code, but instead do a snapshot.

expect(input).toHaveClass('p-inputtextarea p-inputtext p-component p-inputtextarea-resizable');
expect(input.getAttribute('rows')).toBe('3');
expect(input.getAttribute('cols')).toBe('12');

I have not used directly @testing-library/react fireEvent, but use a bit more complicated alternative that allows the testing code to be shared with this storybook play function. This allows also to track the visual snapshots. But anyway in both cases markup snapshots are easier to understand and give you meaningful overview of the full set of changes. They are better than the individual expect() calls, which I think will stop on the first failure and not tell you about all the changes that happened.

Of course snapshots are not a solution for everything and require attention when maintaining, but still I think they are a better choice in many cases, compared to the expect() calls related to markup.

[melloware]

OK this totally rocks. You can absolutely mix and match so in my InputText tests I am using events to simulate typing and then using Snapshot to validate the results and state of the dom. I am a huge fan of this for validating PrimeReact.

[github-actions]

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.