-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Command tool: Enable e2e tests #50833
Conversation
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
'role=option[name="Create a new post \\"E2E Test Post\\""i]' | ||
); | ||
|
||
await page.keyboard.type( 'new page' ); |
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.
Might be good to check that the focus is moved to the input box in the command center.
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.
Why marking this as resolved? 😅
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.
Because I don't think it's that important to spend time on small details like this for e2e tests.
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'm certain that you didn't mean it, but it can feel a bit disrespectful to silence others' comments without an explanation. I don't feel that strongly about this either but at least a comment will help me understand your decision better. ❤️
FWIW, from a reader's perspective, I have no idea what we're typing in when reading this test. It also helps debugging by providing helpful error messages if it fails. In addition, if we had added the test, we might found out that the command center dialog doesn't have an accessible name, which might be an accessibility concern?
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 don't want to be disrespectful but I think we have different opinions on what a review is about. A review is not about validation and more about suggestions and insights that help the PR author land his PR.
That said, I agree that I should have clarified initially that my opinion is that we shouldn't be too finicky with e2e tests and code style and things like that. I think our time in general is better spent on the product.
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 agree that product is our focus, but I think there can also be a good balance for writing elegant e2e tests.
Tests are also part of our code base, which means we apply to them the same standards we apply to all our application code.
Is it readable? Will other contributors be able to understand how our code behaves by looking at its corresponding test?
Quoted from our own doc, not only should we write tests to help catch regressions, but also we should optimize it for others reading the tests. That's why I think accessible selectors and explicit assertions are important.
I hope that I didn't sound "too finicky" about this, just to make it clear, I don't think this is something that needs to be changed. This review is a suggestion. I only left this comment because I didn't understand the code when I first read it, and I was asking for your insights while also providing a simple one-line solution.
I think any kind of discussion is beneficial so that we can reach a consensus, even when we have different opinions, and that's good! After all, communication is oxygen, right 😆 ? Thank you for sharing your opinion in this thread though! 🙂
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 was a bit on the edge last week :). We both want the best for the project, I agree that tests are important and that it's good to have good tests. you're doing great job trying to force us to adopt better tools and guidelines.
On the other side, I do feel (independently from this PR) that globally on the project, developers are giving way too much important to the tooling/testing/... at the expense of the time they should be spending on the product instead. There's a balance to be found and I think we're on the wrong side of the balance at the moment.
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.
Off-topic: It's interesting that I feel the opposite way 😆. I feel like the technical debt in this project has become too significant, and we have too few people working on maintenance and DX is blocking us from implementing product features faster than we should. For instance, npm run dev
is taking around 3~5 seconds for each change on my really fast M1 mac. We still haven't figured out how to hot reload JS or CSS. React devtools and Redux devtools sometimes randomly fail. We're still stuck on Node 14 and npm 6 which already reach EOL, etc.
However, I don't think we should slow down product development. Instead, I think we might need a dedicated team or individual to track these crucial DX issues independently, outside the regular product release cycle. I believe it would eventually enhance our performance and enable us to deliver faster in the long term 🚀 .
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.
A dedicated team might be good but even if a file change takes 1ms or build and tests 80% faster, I don't really think this is what's blocking us from making iterations now. I think we need more time thinking, testing the product and shipping features. We developers (including me at times) tend to focus too much on DevX while the overall impact of that is not that important if we take a look at the broad picture. Writing code and developing is just a tiny part in the time needed to ship a feature (thinking through it, discussing with designers, testing it and getting feedback...), the code is in general the small part for improvements to the production tools to write and ship that code are good but I think we're in the "good enough" state at the moment, not "perfect" but it's not the thing that is hindering iterations on the product.
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.
Well, agree to disagree then 😅.
Faster builds and devtools unblock developers to make iterations faster and try out different approaches. It also help first-time contributors to get started with the project. DX and tests improvements aren't only just about time either. React and Redux devtools are essential tools to help us debug and make sense of the project. They being unreliable is frustrating to us because we'll be forced to use other unfamiliar tools. The Node.js update is also essential for security. Some popular libraries in the community are also starting to drop support for unsupported Node version, and we being stuck on Node 14 is preventing us from leveraging them. I'm not suggesting us to make this project blazing fast to develop, but I don't think it's in a "good enough" state either. Moreover, DX isn't really just about tooling, but also about documentation, following best practices, and solid architecture. I don't want to sound nitpicky or bikeshedding, but the lack of focus of these areas is starting to worry me that we aren't paying enough attention on it 😅.
Flaky tests detected in 9500918. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5044406147
|
Related #48457
What?
Enable e2e tests for the command center now that it's not experimental anymore.