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

Simple select component randomly fails test #343

Closed
lavoscore opened this issue Jun 2, 2023 · 22 comments · Fixed by #369
Closed

Simple select component randomly fails test #343

lavoscore opened this issue Jun 2, 2023 · 22 comments · Fixed by #369

Comments

@lavoscore
Copy link
Contributor

lavoscore commented Jun 2, 2023

I have a very simple QSelect based component that will occasionally fail as image below:

image

It happens very frequently during CI (cypress run), less so via cypress open, and I can't find the cause. The last green message makes me think that Cypress is able to find the popup menu, but even though I can always see its options in the test, withinSelectMenu can't find them inside the menu, failing here:

cy.get('.q-item[role=option]').contains(value).click();

Why would that happen only occasionally? Since withinSelectMenu "assumes there's a single select menu open at any time", I wonder if there could be some extra empty popup left open somewhere, maybe not correctly unmounted by Cypress between tests or something. I'd appreciate advise on how to narrow this bug down further.

Test:

const tipo = ref('PR');

it('v-model works', () => {
  cy.mount(SeletorDeTipo, { props: { ...vModelAdapter(tipo) } });

  cy.dataCy('campo-tipo').closest('.q-select').select('PLL');
  cy.then(() => expect(tipo.value).to.eq('PLL'));
});

Component:

<template>
  <q-select
    label="Tipo"
    v-model="tipo"
    :options="tipos"
    clearable
    data-cy="campo-tipo"
  />
</template>

<script setup lang="ts">
import { computed } from 'vue';

const props = defineProps<{
  modelValue: string | undefined;
}>();

const emit = defineEmits<{
  (e: 'update:modelValue', valor: string | undefined): void;
}>();

const tipos = ['PLL', 'PDL', 'PR', 'IND']

const tipo = computed({
  get: () => props.modelValue,
  set: (valor) => emit('update:modelValue', valor),
});
</script>

I'm using version 5.1 and Cypress 12.11.0.

@david-mohr
Copy link

david-mohr commented Sep 11, 2023

I'm seeing the same issue with the example test for QuasarSelect. As mentioned above, it happens more frequently during cypress run, and rarely if it's on its own (using cypress run -s).

I've found a potential workaround which is to click on the select twice after mounting (once to open, once to close) and the error hasn't happened since.

@IlCallo
Copy link
Member

IlCallo commented Sep 13, 2023

I wonder if there could be some extra empty popup left open somewhere

This is probably the case, or something is somewhat forcing your select to close when it shouldn't
In some cases it could be due to the animation of another Select or component not having finished their outro animation, but this doesn't seem to be the case

Could you upgrade Vue, Quasar and Cypress deps and let me know if it still occurs?
From what I can see in the image, the q-menu isn't actually visible, which can very likely be the cause of the issue
Maybe a change in the select options or modelValue is closing it somehow?

@david-mohr are you able to replicate this on a clean new project? I'm apparently not able to

@IlCallo
Copy link
Member

IlCallo commented Sep 13, 2023

We just noticed the same problem in our own Quasar UI test suite, we're investigating the problem

@david-mohr
Copy link

@IlCallo yes, I can replicate with a new project. It's inconsistent, but one way I've found to trigger it is to run the test just on QuasarSelect, hit CTRL+C as soon as the second test passes, then run it again

$ yarn test:component:ci -s src/components/__tests__/QuasarSelect.cy.js
...
  QuasarSelect
    ✓ makes sure the select is disabled (116ms)
    ✓ selects an option by content (533ms)
# hit CTRL+C here

$ yarn test:component:ci -s src/components/__tests__/QuasarSelect.cy.js
...
  QuasarSelect
    ✓ makes sure the select is disabled (169ms)
    1) selects an option by content

@n05la3
Copy link
Contributor

n05la3 commented Sep 21, 2023

I checked this out with @IlCallo. In my case, I didn't have any issues initially when running on node 16. I then switched to both 18 and 20 and started experiencing similar random errors. Like 3 fails on every 5 runs. Hours later, I could not get any errors again on either 16, 18 or 20 after several trials. @lavoscore, @david-mohr, could you confirm if switching the node version makes any difference?

@david-mohr
Copy link

@n05la3 I found that switching the node version did NOT make any difference (I got failures in all three). FYI I simply used nvm to switch between versions, e.g. nvm use v16

@lavoscore
Copy link
Contributor Author

Maybe it's too soon to celebrate yet, but after upgrading stuff I'm currently unable to reproduce the original behavior. I still see an occasional flake (specific to another test), but had to cypress run a lot to see it. Here's my current dependencies:

"dependencies": {
    "@quasar/extras": "^1.16.3",
    "axios": "^1.5.1",
    "core-js": "^3.30.2",
    "quasar": "^2.12.7",
    "ts-loader": "^9.4.2",
    "vue": "^3.3.4",
    "vue-router": "^4.2.5"
  },
  "devDependencies": {
    "@faker-js/faker": "^8.1.0",
    "@quasar/app-webpack": "^3.11.2",
    "@quasar/quasar-app-extension-testing-e2e-cypress": "^5.1.1",
    "@testing-library/cypress": "^10.0.1",
    "@types/humps": "^2.0.2",
    "@types/node": "^20.1.1",
    "@typescript-eslint/eslint-plugin": "^5.10.0",
    "@typescript-eslint/parser": "^5.10.0",
    "cypress": "^13.3.0",
    "fishery": "^2.2.2",
    "mock-socket": "^9.3.1",
    "prettier": "^2.5.1",
    "typescript": "^5.2.2",
    "workbox-webpack-plugin": "^7.0.0"
  },

Using Node 18.16.0.

Anybody else seeing progress with a similar setup?

@IlCallo
Copy link
Member

IlCallo commented Oct 12, 2023

Which deps have you bumped during your upgrade? And from which previous version?

@haoqunjiang
Copy link

I've looked into this issue a while back but with no luck.

The best I can guess is that it's related to QMenu animations. Because in QMenu.cy.js, all click() calls have waitForAnimations: true enabled, while in QSelect.cy.js, though the component uses QMenu underlyingly, none is called with waitForAnimations: true.

Reference: https://docs.cypress.io/guides/core-concepts/interacting-with-elements#Animations

But this issue is so hard to debug: it never fails on my MacBook Pro with M1 Pro, so I can only test it in GitHub Actions.
https://github.com/sodatea/quasar/commits/test-ci-qselect
I've tried modifying the global Cypress configurations, but the tests still failed randomly.
Without the modified configuration, in Vue Ecosystem CI, the failing test case was always the (prop): emit-value one. After the modification, it became some other random test cases in the same file.

So I can only assume it's related to animations, but never sure how to actually fix it.

@IlCallo
Copy link
Member

IlCallo commented Oct 13, 2023

Thanks for reporting your finding, we'll try playing around with waitForAnimations and see if we can solve this at its root cause

@lavoscore
Copy link
Contributor Author

Which deps have you bumped during your upgrade? And from which previous version? - @IlCallo

dependencies:

"axios": "^1.4.0",
"quasar": "^2.12.0",
"vue": "^3.2.47",
"vue-router": "^4.1.6"

devDependencies:

"@faker-js/faker": "^7.6.0",
"cypress": "^12.11.0",
"@quasar/app-webpack": "^3.9.0",
"@quasar/quasar-app-extension-testing-e2e-cypress": "^5.1.0",
"@testing-library/cypress": "^9.0.0",
"mock-socket": "^9.2.1",
"typescript": "^5.0.4",
"workbox-webpack-plugin": "^6.5.4"

@IlCallo
Copy link
Member

IlCallo commented Oct 20, 2023

I tried to bump deps, but to no avail
I'm able to reproduce the problem with upgraded deps in Quasar tests, but not on Cypress AE tests anymore

Debugging this a bit further, I noticed that Quasar tests throw this error

<div#f_0893ba7e-7776-410a-b65d-d830bc1b73d2_lb.q-menu.q-position-engine.scroll.q-menu--square> is not visible because it has CSS property: visibility: collapse

And this issue on Cypress, which doesn't seem that much related: cypress-io/cypress#21164

@haoqunjiang
Copy link

In this case, it looks like https://github.com/quasarframework/quasar/blob/3ef2a0a036ad7157b1d69c3d0edb4563c625e433/ui/src/utils/private/position-engine.js#L175 isn't being called when the assertion takes place.

@IlCallo
Copy link
Member

IlCallo commented Oct 20, 2023

I was able to get errors consistently by running

yarn test:component:run --spec "src/components/select/__tests__/QSelect.cy.js"

in Quasar repo ui folder

All errors are due to clicks on elements with visibility: collapse
Using waitForAnimations didn't help in any way, but the problem is solved by using force: true on clicks involving .q-menu
I guess this is indeed a problem with QMenu animations, but it doesn't really make sense because Cypress should retry and eventually find the correct element, or at least it used to

The problem is that using force: true isn't possible when doing cy.get('.q-menu').should('be.visible') which considers visibility: collapse as not visible either, and it seems it's not retrying the check either

@IlCallo
Copy link
Member

IlCallo commented Oct 20, 2023

Seems like I pin-pointed the problem to this change: quasarframework/quasar@c9e43fa

Not really sure what's going on there, I'll ask for Razvan input on this

@IlCallo
Copy link
Member

IlCallo commented Oct 20, 2023

@sodatea you're right, it seems like the problem is that it gets stuck into the retry loop here:

https://github.com/quasarframework/quasar/blob/3ef2a0a036ad7157b1d69c3d0edb4563c625e433/ui/src/utils/private/position-engine.js#L120-L125

After 5 retries it just returns without doing anything and this leaves the visibility: collapse in place
By commenting out that check, tests will pass without problems

Not really sure why this happens tho
Apparently, when run in CI mode, cfg.targetEl.offsetHeight or cfg.targetEl.offsetWidth are set to 0 and this breaks the check.
What I find strange is that it doesn't happen consistently, that's on Cypress I guess?

@n05la3
Copy link
Contributor

n05la3 commented Oct 20, 2023

I can confirm the errors on Quasar tests, I'm also consistently getting similar errors. In a few cases, some of the failing test cases pass.

@antonisntoulis
Copy link

I can confirm the issue too, using force: click solves the issue but it's not ideal.

@n05la3
Copy link
Contributor

n05la3 commented Mar 5, 2024

I tried digging into this again. It is difficult to pinpoint the culprit since the issue occurs randomly in CI and rarely locally. But it does happen every now and then. I looked into the code but could not locate anything causing this.

I guess it is related to Cypress finding the element, trying to click on it while it is still invisible. Cypress is supposed to check that the element is visible before clicking but it seems not to be doing so for some reason. Adding explicit visibility checks resolves the issues. I have added these explicit visibility checks to the Quasar QSelect component tests and it seems to no longer be failing.

The issue and Cypress blog post below talk about this more and the recommendation in such cases is to explicitly check that the element is visible before clicking.

@IlCallo
Copy link
Member

IlCallo commented Mar 5, 2024

Could you add it automatically to the ".select" override?

And convert both Quasar tests to use that helper if possible

IIRC they have been written before the helper was available

@n05la3
Copy link
Contributor

n05la3 commented Mar 5, 2024

Yeah, sure. I'll check that out.

@IlCallo
Copy link
Member

IlCallo commented Mar 18, 2024

Seems like this is still happening in some circumstances

In particular, I'm experiencing it with Cypress 13.7.0 and the bundled Electron 27.1.3
It may just be an out of date Electron version causing this

Tests ran against Firefox or Chromium don't generate that issue, and @n05la3 doesn't experience that problem on his machine, so this could be an env-related problem

We'll proceed as defined here and here and then close this issue for good

@sodatea we fixed QSelect and use-validate tests into the main repo already, if you have some time to test them out and remove this workaround

We also recently migrated Quasar monorepo to pnpm, so this should be good to go too: vitejs/vite-ecosystem-ci#208

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 a pull request may close this issue.

6 participants