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

Add e2e tests for inline package #22646

Closed
bvaughn opened this issue Oct 28, 2021 · 42 comments · Fixed by #23019
Closed

Add e2e tests for inline package #22646

bvaughn opened this issue Oct 28, 2021 · 42 comments · Fixed by #23019

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Oct 28, 2021

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
Copy link
Contributor

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

@Biki-das
Copy link
Contributor

Biki-das commented Nov 1, 2021

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

@akgupta0777
Copy link
Contributor

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
Copy link
Contributor

Biki-das commented Nov 1, 2021

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
Copy link
Contributor

akgupta0777 commented Nov 1, 2021

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
Copy link
Contributor

Biki-das commented Nov 1, 2021

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
Copy link
Contributor

I mean the Library isn't decided yet.

@Biki-das
Copy link
Contributor

Biki-das commented Nov 1, 2021

I mean the Library isn't decided yet.

Yeah let bvaughn decide!

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 1, 2021

To be honest, I don't know 😄

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

@Biki-das
Copy link
Contributor

Biki-das commented Nov 1, 2021

Already voted let see! 😀😀

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 1, 2021

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

@akgupta0777
Copy link
Contributor

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

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 1, 2021

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

@Biki-das
Copy link
Contributor

Biki-das commented Nov 1, 2021

ok i would also try to do some work with playwright

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 1, 2021

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
Copy link
Contributor

Biki-das commented Nov 1, 2021

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
Copy link
Contributor

@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.

2021-11-02.12-56-34.mp4

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 2, 2021

@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
Copy link
Contributor

Sure will ask from you.

@akgupta0777
Copy link
Contributor

@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
Copy link
Contributor Author

bvaughn commented Nov 3, 2021

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

@akgupta0777
Copy link
Contributor

@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
Copy link
Contributor Author

bvaughn commented Nov 4, 2021

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

@akgupta0777
Copy link
Contributor

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

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 4, 2021

Just an example test.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 4, 2021

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
Copy link
Contributor

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

@akgupta0777
Copy link
Contributor

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
Copy link
Contributor Author

bvaughn commented Nov 15, 2021

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
Copy link
Contributor

akgupta0777 commented Nov 17, 2021

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

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 17, 2021

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
Copy link
Contributor

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

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 17, 2021

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

@akgupta0777
Copy link
Contributor

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

@akgupta0777
Copy link
Contributor

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

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 18, 2021

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

@akgupta0777
Copy link
Contributor

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
Copy link
Contributor Author

bvaughn commented Dec 9, 2021

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
Copy link
Contributor

akgupta0777 commented Dec 10, 2021

@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
Copy link
Contributor

Let me know if you need some help.

bvaughn pushed a commit that referenced this issue Dec 21, 2021
Builds on top of the existing Playwright tests to plug in the test selector API: https://gist.github.com/bvaughn/d3c8b8842faf2ac2439bb11773a19cec

My goals in doing this are to...
1. Experiment with the new API to see what works and what doesn't.
2. Add some test selector attributes (and remove DOM-structure based selectors).
3. Focus the tests on DevTools itself (rather than the test app).

I also took this opportunity to add a few new test cases– like named hooks, editable props, component search, and profiling- just to play around more with the Playwright API.

Relates to issue #22646
@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 22, 2021

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.)

@bvaughn bvaughn self-assigned this Dec 22, 2021
@Remyelder99
Copy link

Yes

zhengjitf pushed a commit to zhengjitf/react that referenced this issue Apr 15, 2022
Builds on top of the existing Playwright tests to plug in the test selector API: https://gist.github.com/bvaughn/d3c8b8842faf2ac2439bb11773a19cec

My goals in doing this are to...
1. Experiment with the new API to see what works and what doesn't.
2. Add some test selector attributes (and remove DOM-structure based selectors).
3. Focus the tests on DevTools itself (rather than the test app).

I also took this opportunity to add a few new test cases– like named hooks, editable props, component search, and profiling- just to play around more with the Playwright API.

Relates to issue facebook#22646
nevilm-lt pushed a commit to nevilm-lt/react that referenced this issue Apr 22, 2022
Builds on top of the existing Playwright tests to plug in the test selector API: https://gist.github.com/bvaughn/d3c8b8842faf2ac2439bb11773a19cec

My goals in doing this are to...
1. Experiment with the new API to see what works and what doesn't.
2. Add some test selector attributes (and remove DOM-structure based selectors).
3. Focus the tests on DevTools itself (rather than the test app).

I also took this opportunity to add a few new test cases– like named hooks, editable props, component search, and profiling- just to play around more with the Playwright API.

Relates to issue facebook#22646
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants