Skip to content

Refactor shared#4066

Merged
moniika merged 5 commits into
RaspberryPiFoundation:developfrom
moniika:event-click-tests
Jul 24, 2020
Merged

Refactor shared#4066
moniika merged 5 commits into
RaspberryPiFoundation:developfrom
moniika:event-click-tests

Conversation

@moniika

@moniika moniika commented Jul 23, 2020

Copy link
Copy Markdown
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

Part of #4065

Proposed Changes

  • Refactored logic for stubbing Blockly.utils.genUid into shared_helpers.js and updated usages.
  • Refactored helper method for asserting Event values into shared_helpers (assertEventEquals)
  • Created helper methods for assertions on spys for event firing
  • Refactored comment_test.js event_test.js and trashcan_test.js to use new event test helpers
  • Added test for testing emitting UI event for "trashcanOpen"

Reason for Changes

Test Coverage

Ran mocha tests.

Tested on:

  • Desktop Chrome

Documentation

Additional Information

Test methods were refactored as helpers were developed to better fit the needs of multiple tests, causing multiple files to be touched.

#4062 fixes eslint errors on the PR.

Initially after this refactor the test failed when they were running concurrently, revealing that some tests cause events to be fired on a timeout which causes failures in other event tests. A bug was filed #4064 to further investigate and resolve this issue. As a temporary fix, the event queue is cleared when a stub is created for Blockly.Events.fire but it is not the ideal fix.

// TODO(#4064): Remove clearing of event clear here in favor of adding cleanup
// to other tests that cause events to be added to the queue even after they
// end.
Blockly.Events.FIRE_QUEUE_.length = 0;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This fix seems to work.

However, I thought I had tested with this change at some point and it didn't work, but it works now.
I'm a little worried that this fixes most failures, but there's still "sometimes" failures, which is why I want to address this better in a follow up as part of #4064.

@moniika

moniika commented Jul 24, 2020

Copy link
Copy Markdown
Contributor Author

Upon investigation, there are other instances where Blockly.Events.fire is stubbed/spied such as in block_test.js and toolbox_test.js. I plan to address in a follow up PR, to reduce the amount of changes in this PR, but I expect the changes to be very similar to what I've done here.
These other tests I've found that are not stubbing them to fire immediately may also be part of the issue in #4064.

@moniika
moniika merged commit 3868db3 into RaspberryPiFoundation:develop Jul 24, 2020
@moniika
moniika deleted the event-click-tests branch July 24, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants