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

Rewrite widget resizer unit tests #4614

Closed
mlewand opened this issue Aug 19, 2019 · 2 comments · Fixed by ckeditor/ckeditor5-widget#115 or ckeditor/ckeditor5-image#340
Closed

Rewrite widget resizer unit tests #4614

mlewand opened this issue Aug 19, 2019 · 2 comments · Fixed by ckeditor/ckeditor5-widget#115 or ckeditor/ckeditor5-image#340
Assignees
Labels
package:widget type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@mlewand
Copy link
Contributor

mlewand commented Aug 19, 2019

Is this a bug report or feature request?

Task

📋 Steps to reproduce

Currently widget resizer is tested solely by image resizer integration tests.

There are couple of issues with that:

  • Since we're using initEvent, the mouse coordinates gets floored. This means that it's impossible to write reliable tests for hidpi env.
  • It would be great being able to mock (externally trigger) events subscribed by WidgetResizer.
  • We skipped 100% CC, but we want to have it.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-widget Oct 9, 2019
@mlewand mlewand added this to the iteration 27 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". package:widget labels Oct 9, 2019
@mlewand mlewand modified the milestones: iteration 27, iteration 28 Oct 22, 2019
@mlewand
Copy link
Contributor Author

mlewand commented Nov 6, 2019

@Reinmar Just one thing needs to be clarified before we go further:

  • Currently the resize feature is heavily dependent on DOM, should I:
    • Tests things that use DOM heavily (computed rects, events) in a more of integration test-spirit? So pretty much the way it is done for imageresizing and add unit tests for these parts that can be easily tested alone.
    • Mock all the possible dom-related things. This option would require a lot of mocking.

@Reinmar
Copy link
Member

Reinmar commented Nov 6, 2019

Mock mouse movements in a safe and stable way. The rest can come from the browser. The problem with the current tests is that to workaround the hidpi/lowdpi issue you created tests which sometimes give X, sometimes X+1 results which made them far more complex than what they need to be.

Currently, every time when you could've used a simple expected === actual checks for data/model layers, you're doing pattern matching because some width/height values can be either 50px or 51px.

The goal is to make tests simple. Abusing mocks don't make them simple and/or realistic. Agreeing to browser's inconsistencies or the effect of the environment also makes things complex. The choice needs to be pragmatic and based on what can simplify tests as simple and as realistic as they can be.

@Reinmar Reinmar modified the milestones: iteration 28, next, iteration 29 Nov 25, 2019
@Reinmar Reinmar modified the milestones: iteration 29, iteration 30 Feb 13, 2020
Reinmar added a commit to ckeditor/ckeditor5-image that referenced this issue Feb 25, 2020
Tests: Extracted relevant parts of widget resize plugin from the image back to widget plugin. Closes ckeditor/ckeditor5#4614.
Reinmar added a commit to ckeditor/ckeditor5-widget that referenced this issue Feb 25, 2020
Tests: Extracted relevant parts of widget resize plugin from the image back to widget plugin. Closes ckeditor/ckeditor5#4614.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:widget type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
2 participants