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

Round offset to the nearest integer in tests #2142

Closed
wants to merge 2 commits into from
Closed

Conversation

Comandeer
Copy link
Member

What is the purpose of this pull request?

Failing test fix

What changes did you make?

I added Math.ceil to some assertions as Firefox sometimes returns non-integers.

Closes #2141.

@mlewand mlewand self-requested a review June 22, 2018 12:47
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Though it solves the issue on some setups, it still does not work everywhere.

For instance it doesn't pass for me in FF 60.0.2 (64-bit) @ Win 10.

The following gets reported:

Error details for tests/plugins/autocomplete/view

test get caret rect (inline)

Values should be equal.
Expected: 7 (number)
Actual:   8 (number)

test get caret rect with repositioned offset host (classic)

Values should be equal.
Expected: 7 (number)
Actual:   6.8000030517578125 (number)

test get caret rect with repositioned offset host (inline)

Values should be equal.
Expected: 7 (number)
Actual:   8 (number)

@mlewand
Copy link
Contributor

mlewand commented Jun 22, 2018

The feature works correctly, the problem is with tests on FF. I suggest we ignore this for now and fix it correctly in next minor release.

@mlewand mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Jul 18, 2018
@Comandeer Comandeer self-assigned this Sep 3, 2018
@mlewand
Copy link
Contributor

mlewand commented Sep 3, 2018

Some more insights on failing test - it fails only when run from the dashboard. It passes if ran individually.

It depends on screen resolution, it works when window.devicePixelRatio is 1.0 but fails if window.devicePixelRatio is 2.5.

Here's a dump of screen member:

availHeight: 824
​availLeft: 0
​availTop: 0
​availWidth: 1536
​colorDepth: 24
​height: 864
​left: 0
​mozOrientation: "landscape-primary"
​onmozorientationchange: null
​orientation: ScreenOrientation { type: "landscape-primary", angle: 0, onchange: null }
​pixelDepth: 24
​top: 0
width: 1536

@Comandeer
Copy link
Member Author

Comandeer commented Sep 3, 2018

Simple change from Math.ceil to Math.round seems to fix the error. I've also rebased onto latest next.

@mlewand mlewand closed this Sep 12, 2018
@Comandeer Comandeer changed the base branch from next to master September 12, 2018 10:05
@Comandeer Comandeer reopened this Sep 12, 2018
@Comandeer Comandeer added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Jan 16, 2020
@Comandeer Comandeer closed this Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:frozen ❄ The pull request on which work was suspended due to various reasons. review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants