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.

Sign up for GitHub

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

Jump to bottom

Command tool: Enable e2e tests #50833

Merged
merged 3 commits into from
May 22, 2023
Merged

Command tool: Enable e2e tests #50833

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a6190b
Command center: Enable e2e tests
youknowriad May 22, 2023
4f2c6fd
Changes per review
youknowriad May 22, 2023
9500918
Tweaks
youknowriad May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 14 additions & 20 deletions test/e2e/specs/site-editor/command-center.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,36 +17,30 @@ test.describe( 'Site editor command center', () => {
await admin.visitSiteEditor();
} );

test.skip( 'Open the command center and navigate to the page create page', async ( {
test( 'Open the command center and navigate to the page create page', async ( {
page,
} ) => {
await page
.getByRole( 'button', { name: 'Open command center' } )
.focus();
await page.keyboard.press( 'Meta+k' );
const newPageButton = page.locator(
'role=option[name="Create a new page"i]'
);
await expect( newPageButton ).toBeVisible();

// Type a random post title
await page.keyboard.type( 'E2E Test Post' );
await page.click(
'role=option[name="Create a new post \\"E2E Test Post\\""i]'
);

await page.keyboard.type( 'new page' );
Copy link
Member

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.

Copy link
Member

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? 😅

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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! 🙂

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

@youknowriad youknowriad May 29, 2023 •
edited
Loading

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.

Copy link
Member

@kevin940726 kevin940726 Jun 1, 2023 •
edited
Loading

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

await page.getByRole( 'option', { name: 'Add new page' } ).click();
await page.waitForSelector( 'iframe[name="editor-canvas"]' );
const frame = page.frame( 'editor-canvas' );
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
const postTitleInput = frame.locator(
'role=textbox[name=/Add title/i]'
);
await expect( postTitleInput ).toHaveText( 'E2E Test Post' );
await expect(
frame.getByRole( 'textbox', { name: 'Add title' } )
).toBeVisible();
} );

test.skip( 'Open the command center and navigate to a template', async ( {
test( 'Open the command center and navigate to a template', async ( {
page,
} ) => {
await page.keyboard.press( 'Meta+k' );

await page
.getByRole( 'button', { name: 'Open command center' } )
.click();
await page.keyboard.type( 'index' );
await page.click( 'role=option[name="index"i]' );
await page.getByRole( 'option', { name: 'index' } ).click();
await expect( page.locator( 'h2' ) ).toHaveText( 'Index' );
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
} );
} );