-
Notifications
You must be signed in to change notification settings - Fork 7
Playwright component testing #588
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
Conversation
bedrich-schindler
commented
Feb 15, 2025
591decf
to
92827da
Compare
@literat, this PR might be interesting for you as it shows Playwright Component Testing in action I talked about on Wednesday. As we sometimes share some concepts between React UI and Spirit, do not hesitate to add some comments to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really, really like this!
37d95b4
to
17bc13c
Compare
I rebased it as there were conficts in package-lock.json |
@mbohal and @adamkudrna, ready for round 2. |
One more thing. I created follow up for better organization of test runners that way beyond this pull request: Part of the issue is uniform environment to prevent unexpected errors on Apple Silicon where you need to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests alone look good, however I have some major comments on the dev stack and the docs.
No matter that, having finally the visual tests is so cool! 👏🏻
...ents/Alert/__tests__/Alert.spec.jsx-snapshots/Alert-visual-color-danger-1-chromium-linux.png
Outdated
Show resolved
Hide resolved
b9c42a1
to
bf48d15
Compare
As I mentioned in comments, there is follow-up issue that tracks improvements that need to be discussed and processed on Monday's call: #590 (comment) If there is not blocking problem, we should close this as soon as it gets and work on improvements later not to block further work as none of the comment is related to writing tests but to stack around it. Anyway, I thank @adamkudrna for the discussions he raised. I can sound uncompromising, but I must balance between the things that are necessary and that are not to not to block further work and to keep the scope as tight as possible as there are thousands of things that might be solved in this PR. |
cd06c72
to
3d92dc6
Compare
Pull request should be ready for next round @adamkudrna. I also told @lukasbriza to check the discussions again to bring next opinion into discussion. |
TESTING.md
Outdated
by using the following command: | ||
|
||
```bash | ||
npm run test:playwright-ct:show-report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have written this to a comment under outdated yet unresolved discussion. I will put it here as well, just to be sure it does not disappear.
To serve the report I just run cd playwright-report
and then python -m http.server 8000
.
It then serves the report on port 8000.
I find this better:
- port is set inline, no need for the
PW_REPORT_PORT
const and non-pretty code inpackage.json
- better separation of concerns as playwright creates a report and then another tool serves it
- On *NIX systems Python is typically installed by default, Node.js is not.
I think we should add this method to docs. I would even consider removing the current one, but I have no strong feelings about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to make it working. I was able to remember that there is actually problem when running docker compose run
. The problem is that docker compose run
does not expose ports by design and to expose them, --service-ports
must be used. That was reason that it was not working before. It is working now and port can be changed using COMPOSE_PLAYWRIGHT_REPORT_PORT
. I use default ports 9323 as it is not easy to use read variables from multiple env files (.env + .env.playwright). So it now can be served from Docker!
TESTING.md
Outdated
|
||
Test report cannot be server from Docker container yet. | ||
|
||
[gh-gg-node-shell]: /docs/contribute/general-guidelines#node-shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These links do not work outside of the context of mkdocs. We need to add src
to the path.
I dislike having this text twice in the repo, but this also goes for CONTRIBUTING.md
so I think it it beyond the scope of this issue -> I will raise it elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbohal, there was error in CONTRIBUTING.md. Instead of:
there was:
That resulted in error as there is |
4837c85
to
c7c244d
Compare
For some reason, there is 1 requested change, but Github does not show which one. Probably error in the same manner I already reported to Microsoft last week. The only option is to dismiss review to get rid of error.
c7c244d
to
276f39e
Compare
I merged TypeScript support for Playwright into this branch/pull request, squashed commits and rebased it. @lukasbriza, make all changes from this branch (not from |
@adamkudrna, when you have time, you can review this. Without your permission, we would not be able to merge it as you are codeowner. |
I introduced Playwright Component Testing as replacement for Jest and RTL. Those tests must be run within `playwright` docker container with pre-installed browsers to ensure uniform testing environment. `.env.playwright` is created using `postinstall` script to follow zero-config setup. `.env.playwright` can be used to tweak parameters for local development if needed. `.env` newly contains Playwright related configuration for Docker. Currently, only tests for `Alert` and `Button` components are migrated to Playwright CT as this commit is supposed to be proof-of-concept. Rest should be migrated in later pull request. All visual tests use prop tests concept we already have for Jest to simplify testing and to make it DRY. `mixPropTests()` is introduced to mix those to create combinations to be tested. New Github workflow is introduced to test only Playwright as it can be time exhausting. Further improvements are expected to be make in the future.
Transform Playwright related files to Typescript and introduce temporary configuration files `.eslintrc-ts`, `jest.config-ts.js` and `tsconfig.json` that must be removed when whole package is transformed into Playwright. Due to missing TypeScript support in the rest of the package, `*.spec.tsx` and `*.story.tsx` file do not have complete types and might not be compatible with strict type checking that should be introduced in #394. The reason for such change is parallel work on transformation to TypeScript and on Playwright tests.
a7d1e91
to
7c32953
Compare