Skip to content

chore: fix a bug in flaky move test #720

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

Merged
maribethb merged 8 commits into from
Sep 9, 2025
Merged

chore: fix a bug in flaky move test #720

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
681ad1a
chore: fix a bug in flaky move test
maribethb Sep 5, 2025
09cb7f1
chore: add assert to make sure context menu is selected
maribethb Sep 5, 2025
6a474f7
chore: try using keyboard to start move
maribethb Sep 5, 2025
c906f12
chore: lint
maribethb Sep 6, 2025
56df253
Revert "chore: lint"
maribethb Sep 8, 2025
95cdb69
Revert "chore: try using keyboard to start move"
maribethb Sep 8, 2025
ab920dd
fix: add class to move indicator bubble
maribethb Sep 8, 2025
190c3d9
chore: minor refactor
maribethb Sep 9, 2025
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
1 change: 1 addition & 0 deletions src/move_indicator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export class MoveIndicatorBubble
{},
workspace.getBubbleCanvas(),
);
this.svgRoot.classList.add('blocklyMoveIndicatorBubble');
const rtl = workspace.RTL;
Blockly.utils.dom.createSvgElement(
Blockly.utils.Svg.CIRCLE,
Expand Down
57 changes: 37 additions & 20 deletions test/webdriverio/test/move_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,15 @@ suite('Move start tests', function () {
// and block connected to selected block's next connection.
const info = await getFocusedNeighbourInfo(this.browser);

chai.assert(info.parentId, 'selected block has no parent block');
chai.assert.exists(
info.parentId,
'selected block should have parent block',
);
chai.assert(
typeof info.parentIndex === 'number',
'parent connection index not found',
'parent connection index should exist and be a number',
);
chai.assert(info.nextId, 'selected block has no next block');
chai.assert.exists(info.nextId, 'selected block should have next block');

// Start move using keyboard shortcut.
await sendKeyAndWait(this.browser, 'm');
Expand All @@ -59,12 +62,12 @@ suite('Move start tests', function () {
// next/previous connections, and same thing connected to value
// input.
const newInfo = await getFocusedNeighbourInfo(this.browser);
chai.assert(
newInfo.parentId === null,
chai.assert.isNull(
newInfo.parentId,
'moving block should have no parent block',
);
chai.assert(
newInfo.nextId === null,
chai.assert.isNull(
newInfo.nextId,
'moving block should have no next block',
);
chai.assert.strictEqual(
Expand Down Expand Up @@ -106,34 +109,48 @@ suite('Move start tests', function () {
// and block connected to selected block's value input.
const info = await getFocusedNeighbourInfo(this.browser);

chai.assert(info.parentId, 'selected block has no parent block');
chai.assert.exists(
info.parentId,
'selected block should have parent block',
);
chai.assert(
typeof info.parentIndex === 'number',
'parent connection index not found',
'parent connection index should exist and be a number',
);
chai.assert.exists(
info.valueId,
'selected block should have child value block',
);
chai.assert(info.valueId, 'selected block has no child value block');

// Start move using context menu (using keyboard nav).
await sendKeyAndWait(this.browser, [Key.Ctrl, Key.Return]);
await sendKeyAndWait(this.browser, 'm');
await keyDown(
this.browser,
(await contextMenuItems(this.browser)).findIndex(({text}) =>
text.includes('Move'),
),

// Find how many times to press the down arrow
const index = (await contextMenuItems(this.browser)).findIndex(({text}) =>
text.includes('Move'),
);
chai.assert.isAbove(
index,
-1,
'expected Move to appear in context menu items',
);
await keyDown(this.browser, index);
await sendKeyAndWait(this.browser, Key.Return);

// Wait for the move icon to appear so we know we're in move mode.
const bubble = this.browser.$('.blocklyMoveIndicatorBubble');
await bubble.waitForExist();
Copy link
Collaborator

@cpcallen cpcallen Sep 9, 2025

Choose a reason for hiding this comment

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

If bubble isn't used again then why save it? (Unless line length?)

Suggested change
const bubble = this.browser.$('.blocklyMoveIndicatorBubble');
await bubble.waitForExist();
this.browser.$('.blocklyMoveIndicatorBubble').waitForExist();

Copy link
Collaborator Author

@maribethb maribethb Sep 9, 2025

Choose a reason for hiding this comment

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

Good point, I had remembered running into issues with doing this previously but it was when I needed to assert something about bubble which I'm not doing in this case. Fixed (had to add await so didn't commit your suggestion but fixed it locally and pushed)


// Check that the moving block has nothing connected it its
// next/previous connections, and same thing connected to value
// input.
const newInfo = await getFocusedNeighbourInfo(this.browser);
chai.assert(
newInfo.parentId === null,
chai.assert.isNull(
newInfo.parentId,
'moving block should have no parent block',
);
chai.assert(
newInfo.nextId === null,
chai.assert.isNull(
newInfo.nextId,
'moving block should have no next block',
);
chai.assert.strictEqual(
Expand Down