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

test(QDialog): add QDialog tests #16636

Merged
merged 10 commits into from
Feb 23, 2024

Conversation

n05la3
Copy link
Contributor

@n05la3 n05la3 commented Nov 27, 2023

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on an Electron app
  • Any necessary documentation has been added or updated in the docs or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to start a new feature discussion first and wait for approval before working on it)

Other information:

Copy link

github-actions bot commented Nov 27, 2023

UI Tests Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
2 files    0 ❌
2 errors

For more details on these parsing errors, see this check.

Results for commit e26f9ee.

♻️ This comment has been updated with latest results.

Copy link
Member

@IlCallo IlCallo left a comment

Choose a reason for hiding this comment

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

Really nice work!
Aside some dead code around and a couple readability fixes, everything seems fine

ui/src/components/dialog/__tests__/DialogWrapper.vue Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/DialogWrapper.vue Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
@n05la3
Copy link
Contributor Author

n05la3 commented Nov 28, 2023

I have resolved the issues with the PR.

As concerns the use-model-toggle, it was a race condition that caused the issues. After digging, I noticed inconsistencies between the tests and the code when testing the hide event. The hide function in QMenu.js looks something like this:

function handleHide (evt) {
  hidePortal() // The dialog is hidden

  // Some code here. It's how fast this happens that determines if the test will pass or fail

  emit('hide', evt)
}

While in the test looks something like this:

cy.dataCy('method-hide')
  .click({ force: true })
cy.dataCy('menu')
  .should('not.exist')
  .then(() => {
    expect(fn).to.be.called
  })
  • First, there's no guarantee that the click is completed before checking if the menu no longer exists, this therefore creates a race condition and depends on how fast the hide method resolves. Normally, it should check that the menu existed before running the hide method. Otherwise, it will just depend on how fast things happen to pass or fail. In addition to this problem:
  • Checking if menu no longer exists is not the right thing to do because the hidePortal gets called the hide event is emitted. So, the menu may disappear before the event is emitted. This should have been passing because things were happening so fast.

With the QDialog, it mainly needed to ensure that the dialog existed before continuing the tests. A few tests were flaky too and only passed depending on how fast things were happening. We had tests like this:

 cy.dataCy('dialog-page').then(() => {
 1)   cy.dataCy('input-field').focus()
 2)  model.value = true
  
    cy.withinDialog(() => {
      closeDialog()
    })
    cy.dataCy('input-field').should('have.focus')
})

It has a similar problem to the q-toggle-model. When run (1), we are not sure whether or not it succeeds. For instance, in some cases, after running (2), the backdrop will cover the page before completing the focus. So (1) may silently fail and the tests after that are false positives. Instead, this has been replaced with the more stable counterpart below:

cy.dataCy('input-field')
    .type('Hello')
  cy.dataCy('input-field')
    .should('have.value', 'Hello').then(() => {
      model.value = true

      cy.withinDialog(() => {
        closeDialogViaBackdrop()
      })
      getHostElement()
        .should('not.exist').then(() => {
          cy.dataCy('input-field')
            .should('have.focus')
        })
    })

I have made similar changes to the rest of QDialog tests to improve stability.

@n05la3 n05la3 force-pushed the test/add-q-dialog-tests branch 2 times, most recently from f981664 to 01c2b82 Compare November 29, 2023 07:17
Copy link
Member

@IlCallo IlCallo left a comment

Choose a reason for hiding this comment

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

I left some comments to highlight some changes I did and explain to which pitfalls you need to pay attention

I didn't complete the refactor, but I marked with a comment up until where I did it
Good luck

ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/QDialog.cy.js Outdated Show resolved Hide resolved
ui/src/components/dialog/__tests__/DialogWrapper.vue Outdated Show resolved Hide resolved

This comment has been minimized.

n05la3 and others added 8 commits February 22, 2024 10:17
… stopping animation on QMenu to fix event issues

- After a dialog has been closed, the hide event is fired only after the duration transitionDuration has elapsed. transitionDuration is a prop passed to use-model-toggle.js and has a default value of 300ms. Hence, we cannot depend on the dialog closing to conclude that the event has been fired. Setting transitionDuration to 0 ensures it fires immediately
@IlCallo IlCallo merged commit a98dfb3 into quasarframework:dev Feb 23, 2024
2 checks passed
Copy link

Build Results

JSON API

📜 No changes detected.

Types

📜 No changes detected.

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.

None yet

2 participants