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

Proof of Concept for E2E tests using playwright #22754

Merged
merged 4 commits into from
Nov 15, 2021

Conversation

akgupta0777
Copy link
Contributor

Summary

Detailed discussion here : #22646
This PR is just a starting point for writing E2E tests for inline package with playwright.

What does the test do ?

It checks the components(ToDo List) and emulates it in the devtools by clicking and inspecting.
Then it matches the props with component props.

How did you test this change?

run yarn test in react-devtools-inline package.

Quick Demo 👀

POC.mp4

Description of some other changes

Removed hashing on CSS styles to write tests easily. This has no impact on the Styling or any visual changes on test apps in react-devtools-shell.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great proof of concept. I left some thoughts. Happy to pick this up and run with it on our side though at any point.

packages/react-devtools-inline/package.json Outdated Show resolved Hide resolved
await expect(text).toEqual(listItemsProps[i]);
}
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love how simple these tests are.

Slightly uncomfortable about how coupled they are with the test shell harness.

Seems like it would be ideal if the e2e tests setup their own test apps (kind of like our unit tests do) and then tested how DevTools interacted with those. That way, changes to the test shell wouldn't accidentally break tests– and we could add new e2e tests without needing to change the test shell in ways that may make it more cumbersome to work for its primary use case (locally iterating).

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great initial proof of concept to get things started. Thank you! <3

@bvaughn bvaughn merged commit ee8f146 into facebook:main Nov 15, 2021
@bvaughn
Copy link
Contributor

bvaughn commented Nov 15, 2021

Oh interesting. When trying to run the new test target locally I see an error:

$ yarn test:e2e
yarn run v1.22.11
$ playwright test --config=playwright.config.js
/react/packages/react-is/index.js:12
export * from './src/ReactIs';
^^^^^^

SyntaxError: Unexpected token 'export'
    at wrapSafe (internal/modules/cjs/loader.js:984:16)
    at Module._compile (internal/modules/cjs/loader.js:1032:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/react/node_modules/@playwright/test/node_modules/pretty-format/build/plugins/ReactElement.js:8:39)
    at Module._compile (internal/modules/cjs/loader.js:1068:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/react/node_modules/@playwright/test/node_modules/pretty-format/build/index.js:25:44)
error Command failed with exit code 1.

Any suggestions, @akgupta0777?

In case it's relevant:

$ node -v
v14.17.0

@akgupta0777
Copy link
Contributor Author

@bvaughn can you give me more context about what you are trying to do
What are you exporting?

For some reason if you use ES6 syntax with playwright you get an error. Will investigate further into this.

I also got this kind of issue when I tried to import clipboardy(npm package) for testing clipboard functionality of devtools.

@bvaughn
Copy link
Contributor

bvaughn commented Nov 16, 2021

Nothing. Literally just tried to run the new test target you added: yarn test:e2e

@akgupta0777
Copy link
Contributor Author

akgupta0777 commented Nov 16, 2021

You have to build the inline package and start the shell Webpack dev server by running yarn start in devtools-shell

Then run yarn test:e2e in inline package to start tests.

@bvaughn
Copy link
Contributor

bvaughn commented Nov 16, 2021

I was running both inline and shell watch processes, so localhost:8080 was serving the test app. This is a syntax error coming from the playwright process. Doesn't seem related.

@bvaughn
Copy link
Contributor

bvaughn commented Nov 17, 2021

Just to be clear, here's what I've done:

yarn install

scripts/release/download-experimental-build.js --commit=main

cd packages/react-devtools-inline
yarn start

# In another shell
cd packages/react-devtools-shell
yarn start

After verifying that localhost:8080 loads okay:

cd packages/react-devtools-inline

 yarn test:e2e
yarn run v1.22.11
$ playwright test --config=playwright.config.js
/Users/bvaughn/Documents/git/react.devtools/packages/react-is/index.js:12
export * from './src/ReactIs';
^^^^^^

SyntaxError: Unexpected token 'export'
    at wrapSafe (internal/modules/cjs/loader.js:984:16)
    at Module._compile (internal/modules/cjs/loader.js:1032:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/Users/bvaughn/Documents/git/react.devtools/node_modules/@playwright/test/node_modules/pretty-format/build/plugins/ReactElement.js:8:39)
    at Module._compile (internal/modules/cjs/loader.js:1068:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/Users/bvaughn/Documents/git/react.devtools/node_modules/@playwright/test/node_modules/pretty-format/build/index.js:25:44)
error Command failed with exit code 1.

This doesn't seem like it has anything to do with the test shell. It's seems to be a syntax error coming from Playwright trying to load uncompiled code.

Can you confirm that you're able to do a fresh checkout of this repository and the above steps work for you, @akgupta0777 ?

@akgupta0777
Copy link
Contributor Author

I can't reproduce the error on my laptop.
Try building devtools again. run yarn build-for-devtools

I want to know why react-is package is interfering
I think there is no import/export related to it in the code.

Will confirm you after I make a fresh react clone and running test in some hours.

@akgupta0777
Copy link
Contributor Author

akgupta0777 commented Nov 18, 2021

@bvaughn I can now confirm that the tests are not working on the latest version of playwright. I did yarn install to install latest playwright and I got the same error as you.

Now I am working on a fix for that. It's an issue with playwright recent changes to the library I guess.

@bvaughn
Copy link
Contributor

bvaughn commented Nov 18, 2021

Thanks @akgupta0777! 🙇🏼 Keep me posted :)

@bvaughn
Copy link
Contributor

bvaughn commented Nov 18, 2021

Now I am working on a fix for that. It's an issue with playwright recent changes to the library I guess.

Excellent. Thank you

@bvaughn
Copy link
Contributor

bvaughn commented Nov 18, 2021

I did yarn install to install latest playwright and I got the same error as you.

If an older version of Playwright works for this, we could also just use the yarn.lock to pin to it.

Doesn't seem ideal but I thought it would be worth mentioning.

@akgupta0777
Copy link
Contributor Author

akgupta0777 commented Nov 19, 2021

It seems like its the issue with playwright but it isn't. Because it worked magically once when I tried on a fresh react clone on the latest version of playwright.
But on my personal clone it's giving error.

@akgupta0777
Copy link
Contributor Author

@bvaughn
So I finally managed to run these tests but the fix is a little sketchy.

What I did:
1.) I deleted the @playwright folder from react/node_modules/ directory
2.) Then cd into react-devtools-inline package and run npm install -D @playwright/test
This will re install the latest version of playwright on inline package node_modules. Whereas previously on doing yarn install on react-devtools-inline will install the playwright on top level node_modules(in react/node_modules).

Why I did this
Actually closely looking on the error stack trace it is coming from the react/node_modules/@playwright/test and I think that playwright should not be on the top it is right now only needed in inline package.

Conclusion
If it works don't touch it 😅😀

@bvaughn
Copy link
Contributor

bvaughn commented Nov 19, 2021

That's an interesting observation. :)

Conclusion
If it works don't touch it 😅😀

Unfortunately this doesn't really work as a solution in this repo, because we can't ask people to yarn install and then remove a directory and CD into another and run a different command etc.

We'll have to get the dependencies setup correctly so this works for everyone (including CI) without extra manual steps.

@eps1lon
Copy link
Collaborator

eps1lon commented Nov 19, 2021

Tests are runnable with #22790.

@akgupta0777
Copy link
Contributor Author

akgupta0777 commented Nov 19, 2021

That's an interesting observation. :)

Conclusion
If it works don't touch it 😅😀

Unfortunately this doesn't really work as a solution in this repo, because we can't ask people to yarn install and then remove a directory and CD into another and run a different command etc.

We'll have to get the dependencies setup correctly so this works for everyone (including CI) without extra manual steps.

Yeah I agree. This is a workaround not a complete solution.

@akgupta0777
Copy link
Contributor Author

Tests are runnable with #22790.

That's some great news.

zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants