-
Notifications
You must be signed in to change notification settings - Fork 16
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 failing tests #127
Fix failing tests #127
Conversation
It's huge different if spy was not called at all or it was called too many times
Back to global config verification in test
1f53d25
to
9b6de22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice it earlier but it looks like we lost 100% test coverage somehow. Could you take a look @sculpt0r ?
tests/integration.js
Outdated
|
||
deleteCkeditorScripts(); | ||
|
||
return createComponent( { editorUrl: basePath + 'ckeditor.js' } ).then( (comp) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return createComponent( { editorUrl: basePath + 'ckeditor.js' } ).then( (comp) => { | |
return createComponent( { editorUrl: basePath + 'ckeditor.js' } ).then( ( comp ) => { |
tests/integration.js
Outdated
const scripts = Array.from( document.querySelectorAll( 'script' ) ); | ||
const ckeditorScripts = scripts.filter( scriptElement => { | ||
return scriptElement.src.indexOf( 'ckeditor.js' ) > -1; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}); | |
} ); |
The gap in coverage appears due to the fact - that now there is no editor created with empty config. Because I added |
|
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍🏻 We can handle tests related to delayed initialization in the dedicated PR.
First of all, I started with defensive
if
on the fragileCKEDITOR
in the observer (main repository). However, not all tests passed.The most damaged makes setup editor from CDN via
editorUrl
props. So I moved it to the last position, so no other tests will fail because of it: 844da89I have a problem with not incoming events from the main editor code, like
ready
and I decided that I change an assertion, so it shows if the event was invoked at all, rather than verify only if it was one time. Because no calls of the spy in the test - means that some event wasn't raised at a deep level, whereas multiple calls tell us about problems in integration commons: c3fc2a1Based debug in karma run, I found that the failing test
should use correct CKEDITOR build
try to load some assets from the different origin that it was set viaeditorUrl
. And it appears that during the test we are deletingwindow.CKEDITOR
but there are still scripts added via 'namespaceloader'. So I decided to remove allckeditor.js
scripts-like along with deleting globalwindow.CKEDITOR
. That fixes the last test assertion problem: fdb6de3 However, I'm still thinking, that if you load CKE4 from the CDN viaeditorUrl
and then, you checkeditorInstance.config.basePath
- it's different. It has good parts, but that may be something worth checking with ckeditor/ckeditor4#4761.I was able to fix the issue with
MutationObserver
in vue tests, thanks to also new config option: https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-observableParent It tells the observer, which node should be checked and in case of changes - determine if there is an CKE4 instance inside. If we provide an immutable element, then observer callback will never be run: a745009Finally, I have to adjust the code for IE 😢 : 9b6de22
Closes #124