cypress-io / cypress

Fast, easy and reliable testing for anything that runs in a browser.
https://cypress.io
MIT License
47.02k stars 3.18k forks source link

Component test using `cypress/react18` behaves differently than `cypress/react` #23699

Open mbiggs-gresham opened 2 years ago

mbiggs-gresham commented 2 years ago

Current behavior

I have a component test that is testing a slate (slatejs.org) editor by simply typing some text. Using the import cypress/react the test works fine. Using the import cypress/react18 the test fails. The issue seems to be something due to it calling setFocus and updating the react state when focus is gained. The test fails as no text is ever entered in to the editor.

Commenting out the setFocus state update means the test passes under react 18.

Desired behavior

The test should pass the same using cypress/react and cypress/react18.

Test code to reproduce

import React, { useState } from 'react';
import { Slate, Editable, withReact } from 'slate-react';
import { mount } from 'cypress/react18';
import { createEditor, Descendant } from 'slate';

const MyComponent = () => {
    const [, setFocused] = useState<true | false>();
    const [editor] = useState(() => withReact(createEditor()));
    const [editorValue, setEditorValue] = useState<Descendant[]>([{
        type: 'paragraph',
        children: [{ text: '' }],
    }]);
    return (
        <Slate
            editor={editor}
            value={editorValue}
            onChange={(newValue) => {
                setEditorValue(newValue);
            }}>
            <Editable
                data-testid="styled-text-input"
                placeholder="Please enter a value..."
                onFocus={(event) => {
                    setFocused(true);
                }} />
        </Slate>
    );
};

describe('MyComponent', () => {
    it('should render text when typed', () => {
        mount(
            <MyComponent />,
        );

        // Check component is in it's default state.
        cy.get('[data-testid="styled-text-input"]').should('be.visible');
        cy.get('[data-testid="styled-text-input"]').should('contain.text', 'Please enter a value...');

        // Brag about some coconuts.
        cy.get('[data-testid="styled-text-input"]').click();
        cy.get('[data-testid="styled-text-input"]').type('I\'ve got a lovely bunch of coconuts');
        cy.get('[data-testid="styled-text-input"]').should('have.text', 'I\'ve got a lovely bunch of coconuts');
    });
});

Cypress Version

10.6.0

Node version

18.3.0

Operating System

macOS 12.4

Debug Logs

No response

Other

React: 18.2.0 Slate: 0.72.8

lmiller1990 commented 2 years ago

Hmm interesting... I started debugging by looking at the React 18 changes: https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#automatic-batching

If I use flushSync, it works:

onFocus={(event) => {
  flushSync(() => {
    setFocused(true)
  })
}} />

I am not entirely sure why this would make it work, though - there really shouldn't be a difference from Cypress's point of view.

Our react18 adapter is 99% the same as react - the only difference is we do what React recommends - all 10 lines of it: https://github.com/cypress-io/cypress/blob/develop/npm/react18/src/index.ts#L28-L40

I wonder if React changed something about how it handles event under-the-hood in React 18... I notice the text box isn't actually a <textarea /> or <input />, but a <div />. I think we need to look at 1) how cy.type works and 2) if React 18 made any changes to their DOM renderer that would cause this incompatibility. useSync allowing the test to pass is a good hint - this is a react-dom specific React 18 change.

lmiller1990 commented 2 years ago

Okay, I got this to pass. Instead of:

cy.get('[data-testid="styled-text-input"]').type('I\'ve got a lovely bunch of coconuts');

Do:

cy.get('[data-slate-node="text"]').type('I\'ve got a lovely bunch of coconuts');

I am fairly sure this is caused by change introduced in react-dom@18, as opposed to our cypress/react18 library, and how they either batch or propagate events. I don't think there's bug in cy.type either... we could dig into this more, but I think this is likely enough to solve your problem?

mbiggs-gresham commented 2 years ago

Hi, Thanks for investigating, I'll take a look at our tests again. We originally noticed this issue because the cypress tests in our main app that use this component stopped working when we switching to true react 18 mode (eg createRoot). We eventually managed to get it typing by doing a cy.get('[data-testid="styled-text-input"]').parent() however we then noticed the text becomes all garbled. The letters seemingly get entered in the wrong order. So i'm not sure if there will still be an issue there.

I'll go back to our app with your suggestions and see if it resolves that too.

mbiggs-gresham commented 2 years ago

Sticking with just the component test for now, I tried your suggestion of using [data-slate-node="text"] for just the typing however i get errors from cypress that data-slate-node='element' is covering it. It seems the click isn't doing anything. I've not used the flushSync.

So if i then use {force: true} to bypass the typing warning for the time being, you'll see from the following video that repeating the test several times over, the letters occasionally don't appear in the correct order.

I'm hitting refresh and sometimes it works and then every now and again it doesn't. This fails alot more frequently when testing the component in our main app.

https://user-images.githubusercontent.com/34435305/189347343-e0ca746b-9ba7-4e8e-b523-c6d7515a52a9.mov

lmiller1990 commented 2 years ago

Re: force: true... seems this is unavoidable, the element really is covered by another (so the error is correct).

Re: garbled order: that's strange - I didn't see the "wrong letter order" bug. I am also unable to reproduce it. Although I think this is still related to an under-the-hood react-dom 18 change (you mentioned React 17 doesn't suffer this), maybe it's something we do need to account for in some way.

I wonder if it's related this this issue: https://github.com/cypress-io/cypress/issues/7306 (which is getting worked on).

To debug this a little more, can you try using the delay option?

cy.get('...').type('...', { delay: 100 })

Curious if this makes a difference. I wouldn't recommend this as a real fix, but more as a way to debug, so we can get to the core issue.

mbiggs-gresham commented 2 years ago

Adding the 100ms delay does seem to make it better for this component test. I tried several times and it didn't get the order of the letters wrong.

When we tested our real component in our full app however we had to go to 400ms for it to work reliably but the real component is doing alot more than this test one.

lmiller1990 commented 2 years ago

Hm this is strange. Nice that you've found a work around, but this is definitely not ideal and seems like a bug.

I will look into this a bit more, but I can't commit to patching this in the near future, since I'm unsure of the scope of changes required.

I find it very strange this only impacts React 18. Something obviously has changed internally - our React 18 adapter code is identical except for using react-dom/client (new, recommended API).

We should try reproducing this with a regular input controlled by React - I've got a sneaking suspicion this is something to do with how react-slate is implemented. Is this a react-slate only bug?

mbiggs-gresham commented 2 years ago

Thanks, that would be appreciated. I did wonder if it was possibly a slate issue. I've been keeping an eye on both repos to see if anyone else reports similar.

lmiller1990 commented 2 years ago

https://github.com/cypress-io/cypress/issues/23589 is similar. Both related to React and Cypress typing really fast, this one is React 17, though.

I don't know how React 18 works internally, but iirc older versions of React (and maybe v18) has a kind of synthetic event system (not just regular DOM events, it wraps or intercepts them in some fashion). I wonder if this is related to that. Regular DOM events are synchronous, which Cypress assumes to be the case - perhaps there is something specific about React that doesn't play well with rapidly fired DOM events.

I wonder if we could write a custom command that does something like typeAndWait where it will not trigger the net character input until the previous one is actually rendered. Something like

Cypress.Commands.add('typeAndWait', (el, str) => {
  let typed = ""
  for (const s of str) {
    cy.get(el).type(s).then(() => {
      typed += s
      cy.get(el).should('have.text', typed)
    })
  }

  return cy.get(el)
})

// usage
cy.get('slate-element').typeAndWait('some coconuts')

Just throwing ideas out there, since it's not entirely clear if this is a bug in Cypress, or just a side effect of a series of assumptions Cypress makes about typing and DOM events (or something entirely different).

churichard commented 1 year ago

I'm also running into this issue with Cypress 12.3, React 18.2, and Slate 0.86.

I've found that a simple workaround is to focus the editor before I type into it:

cy.get('[data-slate-editor="true"]').focus().type('{movetostart}This is a test')
joelmeiller commented 1 year ago

I am running in a similar issue but the focus suggestion would not help. The weird thing is that the focus jumps back to an input element once I try to click on a button and the onClick is not fired 🤔 Was trying everything that came to my mind but nothing helped. Focus, wait, click on something else first (like the body) before clicking on the button. Nothing... I switched back to React 17 and all works just fine. Unfortunately I cannot provide a test repo for this. None the less, please try to dig further. 🙏🏻 This issue is not solved.

lmiller1990 commented 1 year ago

@joelmeiller can you provide a minimal reproduction? I was able to work around it by "slowing down" - I think Cypress is triggering the events too fast (much faster than a human can). If you have a case that cannot be worked around with slowing down the typing, I'd like to debug that.

Also can you share your browser/OS versions?

joelmeiller commented 1 year ago

I use Mac OS 13.1 and tested on the latest Chrome browser version as well as in the Cypress Electron browser. Both fail. Trying now to create a test app with the same behaviour ...

lmiller1990 commented 1 year ago

Thanks, please share your reproduction when you make it.

joelmeiller commented 1 year ago

@lmiller1990 Trickier then I thought but here you go 😅 https://github.com/joelmeiller/cypress-react-18-reproduction

The code is a very simplified version of the productive private version that I cannot share but that's why things might look a bit overcomplicated. However the behaviour is exactly the same and with React 17 all good with React 18 the button click does not trigger the submit.

lmiller1990 commented 1 year ago

Thank you for this. I will try and get this investigated by the end of Q1. I'm sorry for the slow response, we've got a lot of issues to triage and fix.

iammerrick commented 10 months ago

I'm also getting errors after upgrade to React 18 for a similar in-house component like slate. Definitely an oddity between the interaction/type speed. Delay bandaids my issue too.