testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.26k stars 466 forks source link

Conflict with symfony's live components #1316

Open AdrienClairembault opened 4 months ago

AdrienClairembault commented 4 months ago

Relevant code or config:

A very simple test case like the one below will break due to some conflicts with the way symfony live components operate.

describe('Synchronize Users', () => {
    it('synchronize user and display a report', () => {
        // Go to the synchronize page
        cy.visit('/admin/synchronize');
        cy.findByRole('region', {
            'name': "Synchronization report"
        }).should('not.exist');

        // Trigger synchronization
        cy.findByRole('button', { name: "Synchronize users" }).click();
        cy.findByRole('region', {
            'name': "Synchronization report"
        });
    });
});

When the testing library attempt to compute the DOM (to display it as debug information), it accidentally trigger mutliple invalid backend requests towards the symfony server:

image

Problem description:

The problem is caused by a feature of a peer dependency: pretty-format. This dependency is able to recursively pretty print both DOM nodes and complex javacript variables.

To support complex javascript variable, pretty-print has a specific callToJSON feature that will try to call a toJSON method if it exists on ALL items it is recursively iterating on. I suppose this is mainly useful to support the native implementation Date.prototype.toJSON and allow users to add this method on their objects if need be.

This feature is what leads to the issue, following this code path:

Suggested solution:

The solution proposed in #1315 is to disable the callToJSON feature of the pretty-format dependency using its configuration options.

I am not very familiar with the pretty-format library but it seems very powerful and a lot of its feature are meant to handle complex javascript structures.

Thus, callToJSON is one of these advanced features that seems useless to me if you are only doing DOM pretty printing, which I belive is how it is used by @testing-library/dom (correct me if I'm wrong).

With this in mind, disabling this feature would fix the conflicts with symfony forms without breaking anything as it is (hopefully) irrelevant for DOM printing.

I suppose it could also lead to very small performances improvement as disabling this feature remove a few checks for each DOM node that we iterate over (maybe it could be impactful on huge DOMs ?).

github-actions[bot] commented 4 months ago

Uh oh! @AdrienClairembault, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

eps1lon commented 3 months ago

I think it makes more sense to let users configure the pretty-format options globally. We're going over these again and again and I think there's no good one-size-fits-all. I'd definitely prefer if we call toJson by default since it's not expected that this triggers side-effects. But if a library wants to do that, I'd rather have users explicitly opt-out of it

MatanBobi commented 3 months ago

I think it makes more sense to let users configure the pretty-format options globally. We're going over these again and again and I think there's no good one-size-fits-all. I'd definitely prefer if we call toJson by default since it's not expected that this triggers side-effects. But if a library wants to do that, I'd rather have users explicitly opt-out of it

This was the approach I was leaning towards too. We can pass option to pretty-print instead of going with a "this" or "that" approach.

bretrzaun commented 1 month ago

Any news on this? Just stumbled across this issue myself.