graphql / graphiql

GraphiQL & the GraphQL LSP Reference Ecosystem for building browser & IDE tools.
MIT License
15.81k stars 1.69k forks source link

Overhaul @graphiql jest/RTL suites #2602

Open acao opened 1 year ago

acao commented 1 year ago

The unit test suite for graphiql was already very outdated when work was started in 2.x, throwing warnings and noise. There are many other issues, for example the almost monolithic GraphiQL component test file @thomasheyenbrock mentions in comments

iterative approaches like both suggested will be ideal

(Rewrote ticket for the conversation)

thomasheyenbrock commented 1 year ago

Some of these were already happening before the refactoring. Moving stuff to vite which outputs a minified build really bloated the warnings.

I would propose to revisit our tests for graphiql and @graphiql/react in general. We currently only have "high level" tests for the GraphiQL component. Most of these tests assert functionality that could be tested more granularly on the provider components. I figure that refactoring and moving the current tests would naturally allow us to remove these warnings piece by piece.

acao commented 1 year ago

@thomasheyenbrock indeed:

This warning or a similar one has been happening for some time even before @thomasheyenbrock began work

also agreed about revisiting the tests, they were almost direct ports to the original RTL from an enzyme suite that we implemented when we migrated to jest from mocha and i think the original react testing library, but the high level GraphiQL tests have always been a smell, and have kept us from having better integration level tests. Feel free to consolidate and improve the codemirror mocks as you have, but there will probably be other mocks you'll want to build, and much better testing patterns we can use now that we're using hooks and context (mock contexts for example!)

jonathanawesome commented 1 year ago

I've got a branch locally that has ladle installed in @graphiql/react that's helping me to do some visual component testing and file shuffling/organization. I've also got existing testing code in the prototype that tests a hook through UI components, which could be a nice pattern to use in @graphiql/react.

jonathanawesome commented 1 year ago

Also, I was tripped up for at least a couple of hours because I was unaware that I needed to add exported modules from @graphiql/react to the mock that @graphiql uses. I haven't much exposure to cypress, is it the reason why modules need to be reexported from the mock?

thomasheyenbrock commented 1 year ago

@jonathanawesome the mock file you mentioned is not for Cypress tests but for our unit tests using jest. CodeMirror does not really work in JSDOM, so we mock the editor hooks and components (QueryEditor, useQueryEditor, etc). The rest of @graphiql/react is re-exported as is.

I'm also not super happy with the current state here (it's very easy to forget this). Didn't yet look into how we can selectively mock exports of @graphiql/react "globally", i.e. without needing to do it in all test suites separately. I could imaging that reworking the tests and moving them over to @graphiql/react would reduce the tests we have in graphiql significantly, maybe even completely since we have e2e tests that make sure graphiql as a whole works as expected.