search

SAP / ui5-webcomponents

UI5 Web Components - the enterprise-flavored sugar on top of native APIs! Build SAP Fiori user interfaces with the technology of your choice.
https://sap.github.io/ui5-webcomponents/
Apache License 2.0
1.47k stars 254 forks source link

[Cypress with UI5-WC]: cypress electron version is incompatible with latest version of ui5-wc #9029

Open I585157 opened 1 month ago

I585157 commented 1 month ago

Bug Description

As of https://docs.cypress.io/guides/references/changelog#13-9-0 cypress uses electron 27.3.10. This version of electron doesn't have support for the :dir() global attribute. Given the changes here: https://github.com/SAP/ui5-webcomponents/pull/8241/files#diff-897ff50e6a52c49196793e94fb8dcc2100e38365df98b8195c68a7650d6e9c26 newer version of ui5-wc rely on the :dir() attribute. This reliance results in unexpected functionality in tests for components that rely of the getEffectiveDir() function.

For our tests I've noticed the following components have different behaviors: Menu, Dialog, Carousel. When the electron browser tries to render these components the console has the following error: Uncaught DOMException: Failed to execute 'matches' on 'Element': ':dir(rtl)' is not a valid selector. which points to the getEffectiveDir() function.

Affected Component

No response

Expected Behaviour

Applications using ui5-wc 1.40.0 that are tested with cypress should work as expected when using the default browser cypress provides.

Isolated Example

No response

Steps to Reproduce

  1. Run an application with components mentioned above and ui5-wc 1.24.0 through cypress
  2. observe that functionality is altered when running through electron due to uncaught DOMExceptions

  3. ...

Log Output, Stack Trace or Screenshots

No response

Priority

None

UI5 Web Components Version

1.24.0

Browser

Chrome

Operating System

Mac OS

Additional Context

I've added browser as Chrome, but the browser in this case is the default browser Cypress uses. Cypress plans to upgrade to electron 28 here: https://github.com/cypress-io/cypress/issues/28943. Until this is completed, the default browser won't support :dir(). Please let me know if there is a workaround that can be used until then.

Organization

No response

Declaration

Todor-ads commented 1 month ago

Hello @SAP/ui5-webcomponents-topic-core, Could you please look into this issue?

Regards, Todor

nnaydenow commented 1 month ago

Hi @I585157,

Could you please share more about the setup you are using? Do you use e2e tests or component testing because I'm not able to reproduce the issue in component testing scenario? Which version of cypress you are using because I'm not able to reproduce it in 13.6.4? Also if you are testing a base functionality like setTheme / setLanguage function? And if it's possible to provide isolated example with simple cypress repo would help.

Element.matches(selector) is old functionality supported by all browser and I would expect to return false instead of checking the selector at all.

Regards, Nayden

I585157 commented 1 month ago

Hi @nnaydenow , I have setup a sample react based application using the latest version of ui5-webcomponents, ui5-webcomponents-react, and cypress. Here is the link: https://github.com/I585157/my-app let me know if you have trouble accessing it. To test, please run the app using command npm run start, then run the only e2e spec in the cypress folder (testSpec.cy.js). This spec should navigate to the local application, and then tests if the Carousel is rendered. However, the test fails with the error mentioned above. This is the same issue our e2e scenarios are facing for certain components like Carousel, Dialog, etc.

nnaydenow commented 3 weeks ago

Hi @I585157,

Sorry for the delayed response. I managed to reproduce the issue and you are right that it comes from the Electron version because it uses Chromium version 118, where this CSS selector is not yet available (it was added in version 120). This CSS selector has been supported by all major browsers for over six months, so the problem seems to be with Cypress not keeping their browsers up to date.

I'm not sure what workaround we can use here since we can't update the browser. Is it possible to switch the default browser to Chrome until they do the update?

dkris-sap commented 2 weeks ago

@nnaydenow Unfortunately we cannot switch to Chrome since the automated cypress test execution occurs on Electron and we have no control to switch it to Chrome.

ilhan007 commented 1 week ago

Hi @dkris-sap not sure what we can suggest from our end.

What we or you can do is to file issue to Cypress and explain that they need to update the Electron browser as it lags behind and standard features that are being supported for half an year are breaking with the default browser.

Reverting the change ':dir(rtl)' on our side would be huge and would say impossible as we refactored so much CSS within the components already.

And since you can't change the browser to Chrome, which was the other alternative, I am really out of ideas...

I585157 commented 1 week ago

@ilhan007 , thanks for the suggestions. I've linked this open issue to the cypress epic to upgrade electron (https://github.com/cypress-io/cypress/issues/28943), and created an additional issue with cypress (https://github.com/cypress-io/cypress/issues/29766) until this is resolved.

dkris-sap commented 1 week ago

Hi @dkris-sap not sure what we can suggest from our end.

What we or you can do is to file issue to Cypress and explain that they need to update the Electron browser as it lags behind and standard features that are being supported for half an year are breaking with the default browser.

Reverting the change ':dir(rtl)' on our side would be huge and would say impossible as we refactored so much CSS within the components already.

And since you can't change the browser to Chrome, which was the other alternative, I am really out of ideas...

@ilhan007, Thank you. I understand your position. I guess we'll just wait for the resolution from Cypress.

ilhan007 commented 1 week ago

@dkris-sap @I585157 I wish we could do more, thanks for the understanding!