-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
fix(tests): fix broken browser tests #7324
fix(tests): fix broken browser tests #7324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works on my machine too!
); | ||
chai.assert.equal((await getAllBlocks(browser)).length, i); | ||
await dragNthBlockFromFlyout(browser, 'Align', 0, 250, 50 * i); | ||
chai.assert.equal(i, (await getAllBlocks(browser)).length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be the actual vaue and then the expecte value (swap le arguments).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. My expectations were backwards!
50 * i | ||
); | ||
chai.assert.equal((await getAllBlocks(browser)).length, i); | ||
await dragNthBlockFromFlyout(browser, 'Align', 0, 250, 50 * i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason this needed to be switched out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the basic blocks was breaking (I think it's one that you fixed in dev-tools) and it was making testing difficult.
I want to have a different test to validate that every test block can be shown in the toolbox and dragged in, but this particular test doesn't actually care which block you use.
@@ -182,7 +182,7 @@ suite('Disabling', function () { | |||
110, | |||
110 | |||
); | |||
await connect(browser, child, 'PREVIOUS', parent, 'IF0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I guess this was just getting it close enough that it was working? lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
The test plan specified to use a block from the Basic Blocks category of the test blocks. PR google#7324 changed the test in a way that violated the plan, albeit in aid of a good cause: the basic blocks category had become unusable due to a bug in the test blocks combined with better error checking (in PR google#7289). Since the test blocks were fixed by google/blockly-samples#1774 (and the fix published and Blockly updated by PR google#7313) this violation can be corrected.
The basics
npm run format
andnpm run lint
The details
Resolves
Proposed Changes
DO0
instead ofIF0
)Behavior Before Change
Tests worked for Beka but not for me
Behavior After Change
Hopefully tests work for everyone
Reason for Changes