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.

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 failing in the browser and other test improvements. #6077

Merged
merged 5 commits into from Jul 8, 2019

Conversation

@jansiegel
Copy link
Member

commented Jul 1, 2019

Context

Some tests were only failing in the browser, this change should fix them.

Note: when running tests in the browser, it's important not to do anything and keep the tab focused, as, for example, changing the focus to another window may mess up the focus event recognition, and simply moving the cursor might close the comments pop-up, resulting in a failed test.

Additionally, I modified the ContextMenu plugin and DateEditor to prevent leaving behind containers after destroying the Handsontable instances.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

jansiegel added some commits Jun 27, 2019

- Fix some errors present only in the browser
- Prevent leaving behind unused containers after destroying the Handsontable instances

await sleep(300);

const currentTableWidth = $table.outerWidth();
expect(currentTableWidth).toBeAroundValue($table[0].clientWidth);
expect(currentTableWidth).toBeGreaterThan(initialTableWidth);

done();

This comment has been minimized.

Copy link
@budnix

budnix Jul 1, 2019

Member

done() call is not necessary when async/await is used.

This comment has been minimized.

Copy link
@jansiegel

jansiegel Jul 4, 2019

Author Member

Without done the test would timeout before the async part was finished:
image

After updating Jasmine to the latest version this seem to not happen anymore, so I removed it.

@@ -74,6 +74,7 @@ class DateEditor extends TextEditor {
*/
destroyElements() {
this.$datePicker.destroy();
this.datePicker.remove();

This comment has been minimized.

Copy link
@budnix

budnix Jul 1, 2019

Member

.remove() method is not supported by IE (https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove). Additionally, can I ask you to add/update test which will test if that element was removed?

@@ -265,6 +265,7 @@ class Menu {
this.close();
this.parentMenu = null;
this.eventManager.destroy();
this.container.remove();

This comment has been minimized.

Copy link
@budnix

budnix Jul 1, 2019

Member

.remove() method is not supported by IE (https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove). Additionally, can I ask you to add/update test which will test if that element was removed?

@budnix budnix assigned jansiegel and unassigned budnix Jul 1, 2019

jansiegel added some commits Jul 2, 2019

- Post CR updates
- Update Jasmine to the latest version
- Get rid of the rest of unneeded DOM elements in the test runner
- Fix tests for Firefox
- Fix some tests for higher density displays
- Temporarily disable tests for tables with caption and, fragmentSelection and one tests for date validation (reasons: #6083, #6085)
@jansiegel

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

Updates after the CR:

  • Updated Jasmine to the latest version (2.5.2 -> ^3.4.0)
  • More leftover DOM elements removed
  • Fixed Firefox and retina problems
  • Temporarily disabled some tests - to be re-enabled after fixing #6083 and #6085 (and possibly remove the ones related with table captions)

If I'm not mistaken, these changes seem to have fixed (or at least worked around) the Chrome memory leak problem.

@jansiegel jansiegel assigned budnix and unassigned jansiegel Jul 4, 2019

@jansiegel jansiegel requested a review from budnix Jul 4, 2019

@budnix

budnix approved these changes Jul 5, 2019

Copy link
Member

left a comment

👍

let isValidFormat = moment(valueToValidate, this.dateFormat || dateEditor.defaultDateFormat, true).isValid();
let isValidDate = moment(new Date(valueToValidate)).isValid() || isValidFormat;

This comment has been minimized.

Copy link
@budnix

budnix Jul 5, 2019

Member

A good catch 👍

@budnix budnix assigned jansiegel and unassigned budnix Jul 5, 2019

@jansiegel jansiegel merged commit cab32fe into develop Jul 8, 2019

4 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk - package.json (krzysztofspilka) No new issues
Details

@jansiegel jansiegel deleted the fix-browser-tests branch Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.