Opened 5 years ago

Closed 5 years ago

#12309 closed Bug (fixed)

Test test_getDocumentPosition in core.dom.element.element in unstable

Reported by: Artur Delura Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.5
Component: General Version:
Keywords: Cc:

Description

http://tests.ckeditor.dev:1030/tests/core/dom/element/element#test_getDocumentPosition

On IE11 I've something like this: Expected: 350 (number) Actual: 351 (number)

We should add some acceptance error range.

Change History (11)

comment:1 Changed 5 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 5 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 5 years ago by Artur Delura

Status: assignedreview

Changes in branch:t/12309

comment:4 Changed 5 years ago by Artur Delura

Status: reviewassigned

comment:5 Changed 5 years ago by Artur Delura

Status: assignedreview

Improved tests - changes in the same branch.

comment:6 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed
  1. The assertion function does not check type of expected value.
  2. Instead of swapping min with max better throw an error because if min>max, then it means that the code using assertion is wrong. Swapping leads to a silent error.
  3. You used the assertion function in the wrong way - you mixed expected value with a range in element/element.js

comment:7 Changed 5 years ago by Artur Delura

Status: review_failedassigned

comment:8 Changed 5 years ago by Artur Delura

Status: assignedreview

Changes in the same branch:t/12309.

comment:9 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

comment:10 in reply to:  9 Changed 5 years ago by Artur Delura

Status: review_failedreview

Replying to Reinmar:

  1. tests/utils/assert/isNumberInRange.js - wrong file name

Done.

  1. https://github.com/cksource/ckeditor-dev/commit/c150c3ad8a54145299e8b5722ce50f0287a2f9f0#diff-29fe8c260592a68a6c2e2a5f2dea1f0fL10 - what's this? :P

Removed, to keep consistency with closest environment.

  1. https://github.com/cksource/ckeditor-dev/commit/c150c3ad8a54145299e8b5722ce50f0287a2f9f0#diff-43a3fceb3bf7ceb96aabbe46fd4a9101R93 - you can use assert.isNumber and you won't need to throw errors manually.

Yeach, less code :)

Changes in the same branch:t/12309.

comment:11 Changed 5 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Merged to master with git:b60bb80.

I added one commit which improved tests and made min==max invalid (because in such case assert.areEqual/areSame should be used).

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy