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

Option to disable prettyPrint #1521

Closed
Edhilion opened this issue Apr 28, 2020 · 5 comments
Closed

Option to disable prettyPrint #1521

Edhilion opened this issue Apr 28, 2020 · 5 comments

Comments

@Edhilion
Copy link

What problem does this feature solve?

Hello,

I'm migrating to version beta.33 from beta.11 and, if I understand very well the need of a default pretty print for the wrapper.html(), I still don't understand why the option of disabling the pretty print has been removed in 72f10fb, since it could be set as a default parameter value.

In my particular case, I find it really uneasy to read at tests with '\n' and spaces everywhere. I don't care about how the developers have structured the html, I want to know if the elements are here, in the right order, with the right attributes.

A default value will be a richer feature, without breaking the current behaviour.

Thank you for your attention.

What does the proposed API look like?

html(options = { prettyPrint: true }): string {
  if (options.prettyPrint) {
    return pretty(this.element.outerHTML)
  }
return this.element.outerHTML
}
@dobromir-hristov
Copy link
Contributor

Good question... I have no idea why they removed it, probably because no one thought this would be needed?

Do your tests actually looks worse with proper spacing and indentations?

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 28, 2020

Comparing HTML like that would be extremely fragile. Any tiny change to a class or id would break you tests!

A work around would be some raw DOM API like wrapper.element.outerHTML (or something like that, didn't try it).

@Edhilion
Copy link
Author

@dobromir-hristov @lmiller1990 I have to admit I prefer reading my code as

expect(wrapper.html()).toEqual(
    '<div>' +
    '<div class="very-top">' +
    '<span class="check">check</span> ' +
    '<span class="evaluation">OK</span>' +
    '</div>' +
    '<div data-req-id="req-0" class="req-area req-ok">' +
    'My text' +
    '</div>' +
    '</div>'
);

that I find more "readable" than

expect(wrapper.html()).toEqual(
    '<div>\n' +
    '  <div class="very-top">\n' +
    '    <span class="check">check</span> \n' +
    '    <span class="evaluation">OK</span>\n' +
    '  </div>\n' +
    '  <div data-req-id="req-0" class="req-area req-ok">\n' +
    '    My text\n' +
    '  </div>\n' +
    '</div>\n'
);

Another reason is that some of this code is generated by an external library, and I don't want my tests to break because of a simple indentation change.

@lmiller1990 , when you say "Comparing HTML like that would be extremely fragile", I understand but if I want to be sure that we don't forget a critical evolution (like a class we use) on an external library, I think it is the less worst choice.

I can confirm that wrapper.element.outerHTML is working, but I don't think this kind of "private" fields should be used directly.
I will use it for now, thank you for your time. I hope my request has still its value.

@afontcu
Copy link
Member

afontcu commented Apr 29, 2020

Hi! My two cents:

@lmiller1990 , when you say "Comparing HTML like that would be extremely fragile", I understand but if I want to be sure that we don't forget a critical evolution (like a class we use) on an external library, I think it is the less worst choice.

Not 100% sure what's your use case, but you can assert that some class is properly used with classList or with jest-dom's toHaveClass.

some of this code is generated by an external library, and I don't want my tests to break because of a simple indentation change

Testing the HTML output also leads to a fragile test. A simple attribute reordering would also break the test (a false positive), so you might be "safe" against indentation changes, but not against attribute reordering or similar.

Also, since this is coming from an external library: have you thought about mocking it? This way you are not depending on this library decisions to make or break your tests.

I can confirm that wrapper.element.outerHTML is working, but I don't think this kind of "private" fields should be used directly.

I'd say using this "private" fields (even though they are not!) when testing "private" stuff such as the HTML output makes sense.

Hope it helps!

@Edhilion
Copy link
Author

Thank you @afontcu , this is really interesting in how to think about writing my tests. And in my case, my future tests (it'll be difficult to change all of them right now).

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

No branches or pull requests

4 participants