fix(tests): contextMenuSelect sometimes clicks the wrong block #7485
Conversation
| * have text on it, because we use the text element as the click target. | ||
| * @param itemText The display text of the context menu item to click. | ||
| * @return A Promise that resolves when the actions are completed. | ||
| */ | ||
| async function contextMenuSelect(browser, block, itemText) { |
There was a problem hiding this comment.
In general I think it's easiest if we pass the block element (which has an ID property) around to all of our helpers. This makes the code more consistent, and makes it easier for us to refactor the test helpers if we need to, because it's less likely we have to change test code as well.
There was a problem hiding this comment.
This is true for cases where we're creating the blocks by dragging from the flyout but false for cases where we create blocks by loading from JSON or XML. In the tests above, all of the basic playground tests and some of the delete tests create blocks from the flyout, but the other delete tests load from XML and just use a block ID to find the blocks. As I see it, every time we have a handle on a block we also have a handle on the block's ID, but the opposite is not guaranteed to be true.
Thoughts?
There was a problem hiding this comment.
I think for those cases you can just get the block by ID using our helper for that.
| const clickEl = await getClickableBlockElementById(this.browser, firstBlockId); | ||
| await clickEl.click(); |
There was a problem hiding this comment.
Instead of calling this and then calling click could you create a clickBlock helper that does this and then removes the special ID? Then it cleans up after itself, and I think the code would be easier to read. If the clickBlock helper takes in the same options as click you could also use it from the context menu code.
tests/browser/test/test_setup.js
Outdated
| * Get a clickable element on the block. We can't always use the block's SVG root | ||
| * because clicking will always happen in the middle of the block's bounds | ||
| * (including children) by default, which causes problems if it has holes | ||
| * (e.g. statement inputs). Instead, this tries to get the first text field on the | ||
| * block. It falls back on the block's SVG root. |
There was a problem hiding this comment.
Appreciate the detailed english description here!
tests/browser/test/test_setup.js
Outdated
| * @return A Promise that resolves to the text element of the first label | ||
| * field on the block, or the block's SVG root if no label field was found. | ||
| */ | ||
| async function getClickableBlockElementById(browser, id) { |
There was a problem hiding this comment.
Similarly would like to pass the block here instead of the ID.
rachel-fenichel
changed the title
|
Updated per your suggestions. PTAL. |
The basics
The details
Resolves
Fixes #7484
Proposed Changes
Reason for Changes
Fix issues with clicking when the block's path is occluded or has a hole in it.
Test Coverage
Ran browser tests and watched for unexpected behaviour.
Documentation
I added JSDoc
Additional Information