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

Add IE11 BrowserStack custom launcher. #81

Merged
merged 5 commits into from
Feb 5, 2021
Merged

Add IE11 BrowserStack custom launcher. #81

merged 5 commits into from
Feb 5, 2021

Conversation

sculpt0r
Copy link
Contributor

@sculpt0r sculpt0r commented Jan 18, 2021

Add IE11 BrowserStack custom launcher

Closes #9

Closes #75

@sculpt0r sculpt0r added the status:blocked An issue which development is blocked by another issue (internal or external one). label Jan 18, 2021
@sculpt0r
Copy link
Contributor Author

Blocked by: #75

@github-actions
Copy link

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.

@github-actions github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Jan 26, 2021
@sculpt0r sculpt0r removed the status:blocked An issue which development is blocked by another issue (internal or external one). label Jan 26, 2021
@sculpt0r sculpt0r self-assigned this Jan 26, 2021
@sculpt0r
Copy link
Contributor Author

sculpt0r commented Jan 27, 2021

Seems that CI is green. However, locally few tests are down. I talked with @jacekbogdanski and he points me to ckeditor/ckeditor4-angular#119. However running single Chrome / Firefox stills fail on

SUMMARY:
✔ 43 tests completed
✖ 3 tests failed

FAILED TESTS:
 CKEditor Component
   when "component.value" changes to "foo"
     ✖ should call "instance.setData"
       Chrome 87.0.4280.66 (Linux x86_64)
     AssertError: expected setData to be called with arguments 
         at Object.fail (tests/component.js:30493:25)
         at failAssertion (tests/component.js:30450:20)
         at Object.assert.<computed> [as calledWith] (tests/component.js:30476:17)
         at Context.<anonymous> (tests/component.js:51840:63)

   when "component.readOnly" changes to "true"
     ✖ should call "instance.setReadOnly"
       Chrome 87.0.4280.66 (Linux x86_64)
     AssertError: expected setReadOnly to be called with arguments 
         at Object.fail (tests/component.js:30493:25)
         at failAssertion (tests/component.js:30450:20)
         at Object.assert.<computed> [as calledWith] (tests/component.js:30476:17)
         at Context.<anonymous> (tests/component.js:51840:63)

   when "component.readOnly" changes to "false"
     ✖ should call "instance.setReadOnly"
       Chrome 87.0.4280.66 (Linux x86_64)
     AssertError: expected setReadOnly to be called with arguments 
         at Object.fail (tests/component.js:30493:25)
         at failAssertion (tests/component.js:30450:20)
         at Object.assert.<computed> [as calledWith] (tests/component.js:30476:17)
         at Context.<anonymous> (tests/component.js:51840:63)

@sculpt0r
Copy link
Contributor Author

Rebase onto newest master.

@sculpt0r
Copy link
Contributor Author

After rebase, all local tests are green :)

@github-actions github-actions bot removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Jan 27, 2021
@sculpt0r
Copy link
Contributor Author

Summary:

  • add BrowserStack IE11 launcher
  • failing syntax on IE11 (also failing tests) - fixed with Incorrect files published ckeditor4-integrations-common#15. Now common integration distributes properly transpiled code
  • still failing tests on IE11 fixed with 2f31df4:
    • Object.entries was in usage, but IE doesn't support it
    • Cannot use CKEDITOR.tools for Object.entries or reduce, because we are testing CKEditor namespace loading. There is cached Namespace before we delete window.CKEditor, but inner code of tools stills relay on global CKEditor variable, so it's hard to usage
    • Simplified object iteration: actually we are passing only simple object constructs ({ prop:value}) to fill element props. So there is no need to validate all specific cases like our tools are handling it.

Waiting for CI forever...
Finally, it's ready for review.

@sculpt0r sculpt0r removed their assignment Jan 27, 2021
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

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.

@github-actions github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Feb 4, 2021
@Dumluregn Dumluregn self-requested a review February 4, 2021 17:02
Copy link
Collaborator

@Dumluregn Dumluregn left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻 Only one small thing left - in test code coverage I see that for one browser (I guess it's IE11 but I'm not sure) there is one line uncovered: 108. Could you examine if that's expected or the tests can be improved to check also this condition?

@sculpt0r sculpt0r self-assigned this Feb 5, 2021
@sculpt0r
Copy link
Contributor Author

sculpt0r commented Feb 5, 2021

Indeed - It's IE11...
And it's because watch for value is not fired. That's intentional, but I will check what were the reasons for such ignore flag.

[ {
property: 'value',
value: 'foo',
spyOn: [ 'setData', true ],
ignore: !!CKEditorNamespace.env.ie // (#4)
}, {
property: 'value',
value: '',
spyOn: [ 'setData', false ],
ignore: !!CKEditorNamespace.env.ie // (#4)
}, {
property: 'readOnly',
value: true,
spyOn: [ 'setReadOnly', true ]
}, {
property: 'readOnly',
value: false,
spyOn: [ 'setReadOnly', true ]
}, {
property: 'readOnly',
value: null,
spyOn: [ 'setReadOnly', false ]
} ].forEach( ( { property, value, spyOn: [ method, spyCalled ], ignore = false } ) => {
if ( ignore ) {
return;
}

Here is the reason: #4 

There is also #4 (comment) that we should drop this flag. But now we are also testing with IE11.

IE11 also throws this error (MS admit it's bug).

So, could we leave it as it is?

@github-actions github-actions bot removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Feb 5, 2021
Copy link
Collaborator

@Dumluregn Dumluregn left a comment

Choose a reason for hiding this comment

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

Thanks for checking the coverage thing. I've checked and the flag is still necessary in IE11, so as you wrote we have to leave it. Other than that - 👍🏻

Now let's wait for CI after rebase (I should've rebased before review but well).

@Dumluregn
Copy link
Collaborator

CI is green, so I'm merging the PR 👍🏻

@Dumluregn Dumluregn merged commit d3081f8 into master Feb 5, 2021
@Dumluregn Dumluregn deleted the t/9 branch February 5, 2021 11:43
This was referenced Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IE 11 Compatibility Enable IE11 testing environment
2 participants