E2E tests for Single Product Template #5722
Conversation
sunyatasattva
added
skip-changelog
|
Size Change: +1.29 kB (0%) Total Size: 817 kB
ℹ️ View Unchanged
|
I think this is a good idea @sunyatasattva, thank you for proposing it. If it reduces the scope of this PR and can be done in a follow-up issue I think that is totally fine. At the moment we have zero tests, so if this means we get some tests sooner with a view to iterate then I am all for that! 👍🏻 |
* Add function to wait for Gutenberg canvas * Add alternative ids
sunyatasattva
force-pushed
the
test/5656-fse-single-product
branch
from
e276591 to
b90736c
Compare
| * | ||
| * There are two different possible site editor pages: | ||
| * | ||
| * 1. `themes.php?page=gutenberg-edit-site` is the one used and available if the Gutenberg plugin is enabled. | ||
| * 2. `site-editor.php` is the one available in WP Core. | ||
| * | ||
| * @param {string} query String to be serialized as query portion of URL. | ||
| * @param {'core' | 'gutenberg'} [editorContext='core'] Whether to go to the Gutenberg URL or the Core one. |
There was a problem hiding this comment.
TIL - thanks for all the included insights!
There was a problem hiding this comment.
I was curious and had a look at the util on gutenberg side and the query params seems to be used as JSON object but I've noticed we are using it as url search string.
There is a wrong typing information on the visitSiteEditor util. if we passed query as a string to visitSiteEditor it would fail or at least the query part can produce unexpected results, no? The addQueryArgs used underneath seems to expect an object of query params but then gets mutated into a string with addQueryArgs
visitAdminPage does indeed expect a string. I think we should handle the query to ensure it's compatible with both paths if we want to make both of them available.
There was a problem hiding this comment.
There is a wrong typing information on the visitSiteEditor util. if we passed query as a string to visitSiteEditor it would fail or at least the query part can produce unexpected results, no? The addQueryArgs used underneath seems to expect an object of query params but then gets mutated into a string with addQueryArgs
Good catch! I must have typed it early when I didn't use the query contructor util and then forgot to change it. The woes of not having TS!
visitAdminPage does indeed expect a string. I think we should handle the query to ensure it's compatible with both paths if we want to make both of them available.
I was curious and had a look at the util on gutenberg side and the query params seems to be used as JSON object but I've noticed we are using it as url search string.
Not sure I understand what you mean in those two, could you elaborate?
There was a problem hiding this comment.
Sorry placed the comment marker above implementation because I referred to the comment first. It's about the goToSiteEditor method right under and the error in typing I mentioned above.
We accept query as string but visitSiteEditor if context is gutenberg expects object.
| const SELECTORS = { | ||
| blocks: { | ||
| paragraph: blockSelector( 'core/paragraph' ), | ||
| singleProduct: legacyBlockSelector( | ||
| 'WooCommerce Single Product Block' | ||
| ), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Would it make sense for universal selectors and selector functions above to be part of utils?
We already seem to have some SELECTORS there.
I could imagine this could grow one day to earn its own file :)
There was a problem hiding this comment.
In my opinion and experience with Puppeteer, it's not recommended to have one huge selectors object. In fact, the correct way would be to use the Page Object Pattern, which I am discussing with the Gutenberg fellas (they are actually also agreeing, it's just a matter of a huge refactor).
In this case, my intention is to separate selectors which are universally useful, and selectors which are specific to a test case/test file.
There was a problem hiding this comment.
Had a quick look at Page Object Pattern, looks really interesting. If you had a link to the discussions on this I'd love to learn more!
| */ | ||
| export async function elementExists( selector, root = page ) { | ||
| return !! ( await root.$( selector ) ); | ||
| } |
There was a problem hiding this comment.
would it make sense to split into "general" utils and user flow utils? Ie. repeated user actions related to particular domain.
I liked how you laid it out in Gutenberg repo 🎉 where different domains (ie. site-editor, particular block type etc.) would have their files.
There was a problem hiding this comment.
It does make sense, I agree. Will edit!
* Add empty block theme to mock E2E tests * Install empty theme in the test WP instance
| * | ||
| * @return {Promise<boolean>} Whether the element exists or not | ||
| */ | ||
| export async function elementExists( selector, root = page ) { |
There was a problem hiding this comment.
We could use instead of this the toMatchElement utility that we already have via expect-puppeteer.
Expect an element be present in the page or element.
There was a problem hiding this comment.
@ralucaStan Oh but they are not mutually exclusive. toMatchElement is an assertion, this is not supposed to be an assertion. It can be used for example as a guard clause. Or you can see how it is used here.
There was a problem hiding this comment.
I see, thank you for explaining it to me. The example helped. Maybe it would be good to add a few words about the purpose, so that we don't end up using it in places where toMatchElement might be better. What do you think?
There was a problem hiding this comment.
Yes! I agree, I didn't think about it, I'll add it to my notes.
This PR adds E2E tests for the Single Product Template, and a bunch of test utils that we are going to be able to use in the next tests as well. It also adds an empty theme which supports block editing to the repo to be used with E2E tests.
Specifically regarding #5656, it covers the following test cases:
Closes #5656
Testing
Automated Tests
Manual Testing
How to test the changes in this Pull Request:
trunkon thegutenbergmonorepo, as this PR requires Refactor Site Editor test utils to thee2e-test-utilspackage WordPress/gutenberg#38463 to be there and the changes ine2e-test-utils, while merged on trunk, have not yet been published to npm.npm run test:e2e -- -i "tests/e2e/specs/backend/site-editing-templates.test.js"User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).
@tjcafferkey As I was writing tests for this, I have been thinking more about the conversation in #5656, and I understood a bit more why it felt a bit out of place in this issue to test for the functionality mentioned in point 3.
Basically, I feel that this issue and PR should be more about the Single Product Template, while the adding to basket and so on, is more of a test of the Legacy Single Product Block.
After reading #5346 which you mentioned, I am 100% convinced that it is important that we test certain functionality separately from Woo Core. So, I'm not saying we should add the test case 3 above, but I feel that in the interest of trying to keep PRs small and focused, and iterate fast, it would be best to exclude that from this PR and instead open a new issue with tests for the Legacy Block itself.
What are your thoughts?
If you disagree, I'm totally ok with adding those tests here, but they will definitely require much more work, and perhaps it would be best to spec them in a bit more detailed way and decide what to test within a legacy block.