e2e Tests: Add basic Site Title block coverage #32868
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ockham
added
[Package] E2E Tests
|
Size Change: 0 B Total Size: 1.07 MB ℹ️ View Unchanged
|
I think we should keep them and promote them out of the |
ntsekouras
reviewed
Jun 22, 2021
There was a problem hiding this comment.
Thanks for the tests @ockham ! I had started writing amazingly similar tests 😄 but had cases of intermittent failures like my comment about reload.
I think we shouldn't create the util for setting just yet, as an abstraction would make more sense if we handled more settings pages (general, reading, etc...) and they depend on the. type of the input to get/set values.
🤔 Im not quite sure which makes the most sense either. These tests include the multiple site blocks because we used to be in a state where all site blocks showed up as the same item in the save panel, and saving one would save them all. Thus when we decoupled them we wanted to ensure that saving one did not save others as had previously been the case. Thats not really something we can test with an individual block.
👍 that makes sense! |
ockham
commented
Jun 23, 2021
ockham
changed the base branch from
trunk
to
update/disable-prepublish-buttons-while-saving
ockham
changed the base branch from
update/disable-prepublish-buttons-while-saving
to
trunk
This was referenced Jun 25, 2021
ockham
force-pushed
the
add/site-title-test
branch
3 times, most recently
from
da22fbf
to
61d4970
Compare
swissspidy
reviewed
Jun 29, 2021
swissspidy
reviewed
Jun 29, 2021
swissspidy
reviewed
Jun 29, 2021
ockham
force-pushed
the
add/site-title-test
branch
from
ab3c7a3
to
bfa22c6
Compare
|
One more blocker here: #33072 (comment) 😕 |
Working on a fix over at #33140 |
ockham
force-pushed
the
add/site-title-test
branch
from
bfa22c6
to
6ea8fcc
Compare
|
Going to open for review, since this has been sitting for a while. I had to |
mtias
reviewed
Jul 8, 2021
| await createNewPost(); | ||
| await insertBlock( 'Site Title' ); | ||
| const editableSiteTitleSelector = | ||
| '[aria-label="Block: Site Title"] a[contenteditable="true"]'; |
There was a problem hiding this comment.
Honestly this selector seems possibly less stable than the class name, also could be fragile if we start running tests with i18n.
There was a problem hiding this comment.
I thought that aria-label based block selectors were championed by some as more "user-facing" (and that we thus have a few of those across the codebase), but I don't mind going with the class name. We should probably put some guidelines into writing (if we don't have them already) 😄
There was a problem hiding this comment.
Yes, I'm not sure that's a super robust guideline in practice :)
There was a problem hiding this comment.
I wonder why it's less stable? We should probably stick to one language when running e2e tests. Why would we want to change to another language? If there's something specific in some languages that needs to be tested, we can always special-case it and write our updated queries for that language.
Class names are mostly considered to be implementation details, they could change without notices, and the users could still see the completely same result.
aria-label, contenteditable attributes, and even tag name are also considered implementation details though, but less likely to change. Ideally, we should only rely on actual computed accessible properties, such as role and name. puppeteer-testing-library does exactly that and is being experimented in widgets tests.
At the very least, test ids (data-testid) will still be better than class names. IMO, we should avoid using class names as selectors as much as possible.
ockham
force-pushed
the
add/site-title-test
branch
from
c018077
to
0dc9599
Compare
youknowriad
reviewed
Jul 9, 2021
| await switchUserToTest(); | ||
| } | ||
|
|
||
| const saveEntities = async () => { |
There was a problem hiding this comment.
We're starting to have these multi-entities saving steps in a couple of tests (multi-entities, reusable-blocks...), it looks like we'd probably need to factorize these in some utility.
youknowriad
approved these changes
Jul 9, 2021
ntsekouras
approved these changes
Jul 9, 2021
| } ); | ||
|
|
||
| // FIXME: Fix https://github.com/WordPress/gutenberg/issues/33003 and enable this test. | ||
| // I tried adding an `expect( console ).toHaveErroredWith()` as a workaround, but |
There was a problem hiding this comment.
I guess we should intercept the request on this test. I gave it it try now but needs more work. I will look at it better probably next week.
|
Thanks everyone for reviewing! I'll merge this now and will pass the torch to y'all for the follow-up tweaks that y'all mentioned ❤️ |
|
(FTR, e2e failures are in unrelated e2e test files.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The idea is to add some test coverage for the block's behavior for different access levels, as added by @ntsekouras in #32817 (admin can edit the block contents to change the actual Site Title, editor and lower cannot).
This overlaps a bit with some e2e coverage we already have for the Site Editor, i.e. currently in https://github.com/WordPress/gutenberg/blob/46df095a759809b4b7df5ed776598235a4d751e6/packages/e2e-tests/specs/experiments/multi-entity-saving.test.js, since modifying these settings triggers the multi-entity pre-publish panel. I'm not totally sure yet where that will leave those tests, and whether we should keep them around, or have individual blocks' tests absorb them 🤔 cc/ @Addison-Stavlo
It does seem to make sense to promote them out of the
experimental/subfolder of the e2e tests package, since that particular feature is no longer experimental.How has this been tested?
TODO
[aria-label="..."]rather than implementation ones (e.g. class names).Fixweird 403 error 😕 Edit: For now, find a way to ignore it as expected, and add a comment that Inserting a Site Title block causes a 403 error (REST API) #33003 needs to be fixed in order to drop that ignore.switchUserToAdmin/switchUserToTestlogic.Extract a few helpers into the e2e utils package (such as reading and writing a setting in the 'Settings > General' screen, or multi-entity saving).Edit: Per Nik's comment, let's only do that once they have consolidated a bit, and see wider usage.