facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.18k stars 46.9k forks source link

Add e2e tests for inline package #22646

Closed bvaughn closed 2 years ago

bvaughn commented 3 years ago

DevTools has good unit test coverage but no e2e test coverage. This is because the support for testing extensions is pretty limited in e2e testing libraries. That being said, we could probably get 80% of the value of e2e testing leveraging our react-devtools-inline package.

We should create a few example tests (e.g. load hook names, select a component and edit props/state, etc) to see how viable this is.

For now, these tests should not block PRs from landing.

akgupta0777 commented 3 years ago

Speaking of libraries which one we should use, Cypress or jest-puppeteer for E2E testing ? I think Cypress is a great choice.

Biki-das commented 3 years ago

Would like to work on this, Puppeter would be a good choice for this

akgupta0777 commented 3 years ago

If we use puppeteer we can only test extension on chrome. But with cypress E2E tests can run on firefox,chrome and edge too.

Biki-das commented 3 years ago

If we use puppeteer we can only test extension on chrome. But with cypress E2E tests can run on firefox,chrome and edge too.

Yeah and the docs of cypress is super beginner friendly how about we both work together and make pull request in patches?

akgupta0777 commented 3 years ago

If we use puppeteer we can only test extension on chrome. But with cypress E2E tests can run on firefox,chrome and edge too.

Yeah and the docs of cypress is super beginner friendly how about we both work together and make pull request in patches?

Yeah I have no issues in working together.

But I want opinion on this issue by @bvaughn . Also want some info for a Kickstart.

Biki-das commented 3 years ago

If we use puppeteer we can only test extension on chrome. But with cypress E2E tests can run on firefox,chrome and edge too.

Yeah and the docs of cypress is super beginner friendly how about we both work together and make pull request in patches?

Yeah I have no issues in working together.

But I want opinion on this issue by @bvaughn . Also want some info for a Kickstart.

Well he gave us some, firstly we would have to individually write test for. Hooks, props etc too see if it's viable or not

akgupta0777 commented 3 years ago

I mean the Library isn't decided yet.

Biki-das commented 3 years ago

I mean the Library isn't decided yet.

Yeah let bvaughn decide!

bvaughn commented 3 years ago

To be honest, I don't know 😄

Let's see what others say: https://twitter.com/brian_d_vaughn/status/1455155155830194184

Biki-das commented 3 years ago

Already voted let see! 😀😀

bvaughn commented 3 years ago

So far it seems like a lot of votes for Cypress with some write-ins for Playwright

akgupta0777 commented 3 years ago

I want to do some research on playwright as it might be better option than cypress.

bvaughn commented 3 years ago

Seems pretty promising, yes. I'm interested in Playwright now as well.

Biki-das commented 3 years ago

ok i would also try to do some work with playwright

bvaughn commented 3 years ago

At this point, my vote is for Playwright. If either of you would like to put together a demo/PR that tests the react-devtools-inline package– that would be a great starting point.

Biki-das commented 3 years ago

At this point, my vote is for Playwright. If either of you would like to put together a demo/PR that tests the react-devtools-inline package– that would be a great starting point.

sure will start working on it

akgupta0777 commented 3 years ago

@bvaughn Here is the demo with Cypress. In this demo I am running two tests First one checks there are 3 todo items on the list by default Second one adds a todo item Hello world on the list by typing it on the input.

As it is a demo I haven't emulated the devtools but we can surely do it with cypress.

https://user-images.githubusercontent.com/51379307/139804079-35d2a2b4-fdce-4173-b312-b983dc549a35.mp4

bvaughn commented 3 years ago

@akgupta0777 Thanks for sharing the demo.

As it is a demo I haven't emulated the devtools but we can surely do it with cypress.

Just in case there's any confusion, this is the part I'm specifically interested in 😄

akgupta0777 commented 3 years ago

Sure will ask from you.

akgupta0777 commented 3 years ago

@bvaughn Are these ids attached to inputs and other components dynamic or static ? I rebuild the react-devtools-shell many times but IDs are not changing so can I assume these are static ?

image

bvaughn commented 3 years ago

@akgupta0777 Should probably attach test ids to things we want to have automated tests for.

akgupta0777 commented 3 years ago

@bvaughn what assertion should I make to test props on ListItems. See attached image below for reference image

I am thinking of making an assertion on props exist or not if exist test will PASS and if not it will FAIL. Do you want to suggest any other assertion ?

bvaughn commented 3 years ago

Let's just start with a proof of concept test. The specifics of what it tests isn't super important.

akgupta0777 commented 3 years ago

Uhmm... Proof of concept ? Sorry I don't know what doest that mean. Can you explain a little bit please 😅😅

bvaughn commented 3 years ago

Just an example test.

bvaughn commented 3 years ago

To clarify, I don't have much bandwidth right now to describe/design specific test scenarios. I just filed this task for my team to consider for later.

akgupta0777 commented 3 years ago

Ok I will try my best and report back to you.

akgupta0777 commented 3 years ago

I tried a lot of things, different approaches ,strategies with cypress but it is really very hard to write tests with iframes. I switched to playwright and want to try it and really playwright is a outstanding choice for apps with iframes. It's really a joy plus its very productive. I wrote tests with playwright in a very short time than cypress.

Anyways, I am dropping the Proof of concept. #22754

bvaughn commented 2 years ago

I love the direction we're going with Playwright.

One additional thing we could consider in the future is to use the (still experimental) test selector API for e2e testing purposes. (See #22760.) This would not be as complete a solution as Playwright, but would have the added benefit of dog fooding our own APIs.

akgupta0777 commented 2 years ago

I am keeping an eye on test selector API too. Will try to get involved in development of API .

bvaughn commented 2 years ago

You can now test it out in the react-dom@experimental build by aliasing react-dom to react-dom/testing if you'd like.

Here's a readme: https://gist.github.com/bvaughn/d3c8b8842faf2ac2439bb11773a19cec

akgupta0777 commented 2 years ago

Here's a readme: https://gist.github.com/bvaughn/d3c8b8842faf2ac2439bb11773a19cec Thanks I am about to ask you about this 😄.

bvaughn commented 2 years ago

@akgupta0777 I'm still not able to run #22754 locally. Would appreciate your help if you have any insight.

akgupta0777 commented 2 years ago

I am happy to help and I am on it to find the root cause. Works on my machine though

akgupta0777 commented 2 years ago

running yarn test and yarn test --prod also gives the same error. Tests are failing.

bvaughn commented 2 years ago

@akgupta0777 So e2e tests pass for you, but (regular) unit tests fail with the react-is syntax error?

akgupta0777 commented 2 years ago

No just ignore that it was a mistake I did when trying to fix this.

Actually the problem is test runners like playwright only uses commonjs and react-is is using export. Babel is not transpiling properly I think.

bvaughn commented 2 years ago

Hey @akgupta0777!

Thanks to the fix from @eps1lon, I can run these locally now 🥳

But the third test added in #22754 consistently fails for me: Screen Shot 2021-12-09 at 2 09 09 PM

akgupta0777 commented 2 years ago

@bvaughn I know this problem and I identified it and already fix this in the #22868 . The problem is why it fails is it waits for selector with exact span.Value__tNzum the span.Value part is correct but the next part __tNzum is the hash which is generally different on different OS. But I changed it to span[class^=Value] A regex which matches the span with className starts with Value.

akgupta0777 commented 2 years ago

Let me know if you need some help.

bvaughn commented 2 years ago

Once #23019 lands, we can close this issue out. (It will run the new e2e tests we have in Circle CI automatically with each new PR.)

Remyelder99 commented 2 years ago

Yes