search

ckeditor / ckeditor4

The best enterprise-grade WYSIWYG editor. Fully customizable with countless features and plugins.
https://ckeditor.com/ckeditor-4
Other
5.8k stars 2.48k forks source link

Create helper and unit test for testing focus in dialogs #3706

Open msamsel opened 5 years ago

msamsel commented 5 years ago

Type of report

Task

Provide a description of the task

Task extracted from #3499.

The purpose of the task is to extend the approach used in #3499, to test in a similar way all dialogs available in the editor.

In order to have it, is to detect a when focus changes inside dialog, what is done through custom event registered inside 'onLoad' function, which fires focusChange event, used inside tests. https://github.com/ckeditor/ckeditor4/blob/48c228c004762a66b72f979146e225159eb060aa/tests/plugins/dialog/_helpers/tools.js#L4-L22

Currently, I see 2 ways how it might be done (but maybe there are other ways also). There is dialogDefinition event, where is access to a dialog instance before it is actually created.

  1. There might be written a special onLoad function, which preserves original one, and also add custom one required for the test purpose.
  2. There might be somehow available access to 'load' event, so it could be possible to attach to this event in a similar way as it is done inside dialog: https://github.com/ckeditor/ckeditor4/blob/48c228c004762a66b72f979146e225159eb060aa/plugins/dialog/plugin.js#L349-L350

With that change, it should be easy to create some unit test checking focus behaviour for other dialogs inside the editor, like: link, findandreplace, table, etc.

Other details

jacekbogdanski commented 4 years ago

I'm a bit unsure about the purpose of this task. As I see we already covered unit tests for focus change when working on https://github.com/ckeditor/ckeditor4/pull/3499

Do you suggest that we need:

  1. Additional tests for changes introduced by https://github.com/ckeditor/ckeditor4/pull/3499 to test it on real dialogs?
  2. Additional tests for already existing plugins, so they have better test coverage?
  3. Extend current dialog behavior, so it notifies about focus change between input elements?

It seems for me that for:

  1. If PR passed review and this issue is not treated as a tech dept, I suppose that current unit tests are enough to guarantee code changes correctness?
  2. It's never enough unit tests, but as these plugins already exist and have been tested by us and community against certain use cases, should we spend additional time on research and development for the sake of writing tests?
  3. It seems like a nice feature if it's hard to detect such change without some complicated code, but in that case, the ticket should be marked as feature request.

I could miss the point of this ticket, so please, write more about the purpose if you can.

msamsel commented 4 years ago

Generally is 2nd option. The task was suggested here: https://github.com/ckeditor/ckeditor4/pull/3499#pullrequestreview-325984148

Also I wonder, do you see in benefit in creating more integrational tests (so testing focus on some real plugins dialogs instead of test dialogs)?

Currently, we do not check focus behaviour in dialogs in existing plugins. The idea is to introduce such tests. Now some more complicated cases might not be noticed. For examples:

jacekbogdanski commented 4 years ago

Could you also reflect to:

It's never enough unit tests, but as these plugins already exist and have been tested by us and community against certain use cases, should we spend additional time on research and development for the sake of writing tests?

Shouldn't we spend time writing additional tests only once the issue occurs i.e. when we notice it or some of our users report an issue ticket? By no means, I don't treat it as a waste of time, but I'm not sure if it should have a high priority for us right now, especially that it has been estimated as a medium task.

msamsel commented 4 years ago

@f1ames WDYT? Maybe we should move this task to backlog for now?

f1ames commented 4 years ago

The idea here was as @msamsel mentioned to test focus on dialogs used "in the wild" not mockup dialogs, since they can have some different logic and different handling of some case than the simple mocked dialogs.

Referring to https://github.com/ckeditor/ckeditor4/issues/3706#issuecomment-570163314:

If PR passed review and this issue is not treated as a tech dept, I suppose that current unit tests are enough to guarantee code changes correctness?

That's mostly true. The current unit tests covers all PRs changes, however they use mockup dialogs which doesn't fully reflect the different cases of dialogs we have for all the plugins. OTOH since focus is handled by the base dialog implementation, we assume that this unit tests are enough. Also different dialogs were tested manually and no issues were detected.

There might be some changes in a specific dialog in the future which causes an issue, but since we don't have tests there (and don't plan to cover all dialogs), that's a different story (which could have happened before too).

It seems like a nice feature if it's hard to detect such change without some complicated code, but in that case, the ticket should be marked as feature request.

It's only for testing purposes so more like internal task as it's now. And adding such util would allow us to easily test focus for any dialogs in future.

It's never enough unit tests, but as these plugins already exist and have been tested by us and community against certain use cases, should we spend additional time on research and development for the sake of writing tests?

So the main purpose of this ticket was not to cover different dialogs with unit tests, but to provide an util which will allow testing real dialogs (not mockups) focus.

Anyway, it's in the next milestone, we could discuss it further when planning next iteration also having the context of other priorities then. For now I would leave it there.

jacekbogdanski commented 4 years ago

So the main purpose of this ticket was not to cover different dialogs with unit tests, but to provide an util which will allow testing real dialogs (not mockups) focus.

That makes more sense, thanks for the clarification.

Anyway, it's in the next milestone, we could discuss it further when planning next iteration also having the context of other priorities then. For now I would leave it there.

:+1: