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

fix(tests): contextMenuSelect sometimes clicks the wrong block #7485

Merged
merged 7 commits into from
Sep 13, 2023
Merged

fix(tests): contextMenuSelect sometimes clicks the wrong block #7485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6d63f3c
feat(tests): add helper to get a clickable element on a block
rachel-fenichel Sep 12, 2023
1f5dc25
fix(tests): use new helper function for right-clicks
rachel-fenichel Sep 12, 2023
6a920a8
fix(tests): use new helper function for right-clicks
rachel-fenichel Sep 12, 2023
8d6eb80
chore(tests): use new helper function in delete tests
rachel-fenichel Sep 12, 2023
cad3d4b
chore(tests): format
rachel-fenichel Sep 12, 2023
5ee26c9
feat: use block instead of block ID in getClickableBlockElement
rachel-fenichel Sep 13, 2023
67f42f0
chore(tests): rewrite getClickableBlockElement as clickBlock
rachel-fenichel Sep 13, 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
7 changes: 3 additions & 4 deletions tests/browser/test/basic_playground_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,23 +69,22 @@ suite('Right Clicking on Blocks', function () {
suiteSetup(async function () {
this.browser = await testSetup(testFileLocations.PLAYGROUND);
this.block = await dragNthBlockFromFlyout(this.browser, 'Loops', 0, 20, 20);
this.blockId = this.block.id;
});

test('clicking the collapse option collapses the block', async function () {
await contextMenuSelect(this.browser, this.block, 'Collapse Block');
chai.assert.isTrue(await getIsCollapsed(this.browser, this.blockId));
chai.assert.isTrue(await getIsCollapsed(this.browser, this.block.id));
});

// Assumes that
test('clicking the expand option expands the block', async function () {
await contextMenuSelect(this.browser, this.block, 'Expand Block');
chai.assert.isFalse(await getIsCollapsed(this.browser, this.blockId));
chai.assert.isFalse(await getIsCollapsed(this.browser, this.block.id));
});

test('clicking the disable option disables the block', async function () {
await contextMenuSelect(this.browser, this.block, 'Disable Block');
chai.assert.isTrue(await getIsDisabled(this.browser, this.blockId));
chai.assert.isTrue(await getIsDisabled(this.browser, this.block.id));
});

test('clicking the enable option enables the block', async function () {
Expand Down
25 changes: 7 additions & 18 deletions tests/browser/test/delete_blocks_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const {
testFileLocations,
getAllBlocks,
getBlockElementById,
clickBlock,
contextMenuSelect,
PAUSE_TIME,
} = require('./test_setup');
Expand Down Expand Up @@ -130,15 +131,13 @@ suite('Delete blocks', function (done) {
(await getBlockElementById(this.browser, firstBlockId)).waitForExist({
timeout: 2000,
});
this.firstBlock = await getBlockElementById(this.browser, firstBlockId);
});

test('Delete block using backspace key', async function () {
const before = (await getAllBlocks(this.browser)).length;
// Get first print block, click to select it, and delete it using backspace key.
const block = (await getBlockElementById(this.browser, firstBlockId)).$(
'.blocklyPath',
);
await block.click();
await clickBlock(this.browser, this.firstBlock, {button: 1});
await this.browser.keys([Key.Backspace]);
const after = (await getAllBlocks(this.browser)).length;
chai.assert.equal(
Expand All @@ -151,10 +150,7 @@ suite('Delete blocks', function (done) {
test('Delete block using delete key', async function () {
const before = (await getAllBlocks(this.browser)).length;
// Get first print block, click to select it, and delete it using delete key.
const block = (await getBlockElementById(this.browser, firstBlockId)).$(
'.blocklyPath',
);
await block.click();
await clickBlock(this.browser, this.firstBlock, {button: 1});
await this.browser.keys([Key.Delete]);
const after = (await getAllBlocks(this.browser)).length;
chai.assert.equal(
Expand All @@ -167,8 +163,7 @@ suite('Delete blocks', function (done) {
test('Delete block using context menu', async function () {
const before = (await getAllBlocks(this.browser)).length;
// Get first print block, click to select it, and delete it using context menu.
const block = await getBlockElementById(this.browser, firstBlockId);
await contextMenuSelect(this.browser, block, 'Delete 2 Blocks');
await contextMenuSelect(this.browser, this.firstBlock, 'Delete 2 Blocks');
const after = (await getAllBlocks(this.browser)).length;
chai.assert.equal(
before - 2,
Expand All @@ -180,10 +175,7 @@ suite('Delete blocks', function (done) {
test('Undo block deletion', async function () {
const before = (await getAllBlocks(this.browser)).length;
// Get first print block, click to select it, and delete it using backspace key.
const block = (await getBlockElementById(this.browser, firstBlockId)).$(
'.blocklyPath',
);
await block.click();
await clickBlock(this.browser, this.firstBlock, {button: 1});
await this.browser.keys([Key.Backspace]);
await this.browser.pause(PAUSE_TIME);
// Undo
Expand All @@ -199,10 +191,7 @@ suite('Delete blocks', function (done) {
test('Redo block deletion', async function () {
const before = (await getAllBlocks(this.browser)).length;
// Get first print block, click to select it, and delete it using backspace key.
const block = (await getBlockElementById(this.browser, firstBlockId)).$(
'.blocklyPath',
);
await block.click();
await clickBlock(this.browser, this.firstBlock, {button: 1});
await this.browser.keys([Key.Backspace]);
await this.browser.pause(PAUSE_TIME);
// Undo
Expand Down
59 changes: 47 additions & 12 deletions tests/browser/test/test_setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,50 @@ async function getBlockElementById(browser, id) {
return elem;
}

/**
* Find a clickable element on the block and click it.
* 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.
* @param browser The active WebdriverIO Browser object.
* @param block The block to click, as an interactable element.
* @param clickOptions The options to pass to webdriverio's element.click function.
* @return A Promise that resolves when the actions are completed.
*/
async function clickBlock(browser, block, clickOptions) {
const findableId = 'clickTargetElement';
// In the browser context, find the element that we want and give it a findable ID.
await browser.execute(
(blockId, newElemId) => {
const block = Blockly.getMainWorkspace().getBlockById(blockId);
for (const input of block.inputList) {
for (const field of input.fieldRow) {
if (field instanceof Blockly.FieldLabel) {
field.getSvgRoot().id = newElemId;
return;
}
}
}
// No label field found. Fall back to the block's SVG root.
block.getSvgRoot().id = findableId;
},
block.id,
findableId,
);

// In the test context, get the Webdriverio Element that we've identified.
const elem = await browser.$(`#${findableId}`);

await elem.click(clickOptions);

// In the browser context, remove the ID.
await browser.execute((elemId) => {
const clickElem = document.getElementById(elemId);
clickElem.removeAttribute('id');
}, findableId);
}

/**
* @param browser The active WebdriverIO Browser object.
* @param categoryName The name of the toolbox category to find.
Expand Down Expand Up @@ -428,23 +472,13 @@ async function dragBlockFromMutatorFlyout(browser, mutatorBlock, type, x, y) {
* context menu item.
*
* @param browser The active WebdriverIO Browser object.
* @param block The block to click, as an interactable element. This block must
* @param block The block to click, as an interactable element. This block should
* 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for those cases you can just get the block by ID using our helper for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// 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, we'll click directly on the first bit of text on the block.
const clickEl = block.$('.blocklyText');

// Even though the element should definitely already exist,
// one specific test breaks if you remove this...
await clickEl.waitForExist();

await clickEl.click({button: 2});
await clickBlock(browser, block, {button: 2});

const item = await browser.$(`div=${itemText}`);
await item.waitForExist();
Expand Down Expand Up @@ -515,6 +549,7 @@ module.exports = {
getSelectedBlockElement,
getSelectedBlockId,
getBlockElementById,
clickBlock,
getCategory,
getNthBlockOfCategory,
getBlockTypeFromCategory,
Expand Down