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

Refactored dashboard drag/resize testing #3726

Merged
merged 2 commits into from
Apr 22, 2019
Merged

Refactored dashboard drag/resize testing #3726

merged 2 commits into from
Apr 22, 2019

Conversation

ranbena
Copy link
Contributor

@ranbena ranbena commented Apr 19, 2019

  • Refactor

Description

As preparation for #3722 I leveraged the retrying nature of should() to "wait" till drag/resize animations are over, therefore making these methods much simpler and more generic.

This is a no brainer - all test pass and that's all that matters!


@gabrieldutra I couldn't make increase(func).by(value) work, so instead I just assert the end value (width/height/top/left). It's less pretty but works just as well.

@ranbena ranbena self-assigned this Apr 19, 2019
Copy link
Member

@gabrieldutra gabrieldutra left a comment

Choose a reason for hiding this comment

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

Left some comments from what I've noticed, but it's already a gain from what it was before.

I actually had the mind-blowing of that nature of .should() methods recently as well and here we can see the difference ^^

expect(delta.height).to.eq(0);
});
resizeBy(cy.get('@textboxEl'), 0, 10)
.then(() => cy.get('@textboxEl'))
Copy link
Member

Choose a reason for hiding this comment

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

I think this being necessary is a sign that passing a cy.get() as an argument is an anti-pattern (similar to this one). We should rather use it as a Cypress command or pass something else as argument.

For now I think it's ok to leave as is, but let's keep in mind this to when we discover a better way to treat those helper commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worked on making dragBy and resizeBy into commands but it didn't go smooth so I'm leaving as is to keep moving forward.

client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
@ranbena ranbena merged commit fb48bc3 into master Apr 22, 2019
@ranbena ranbena deleted the drag-resize-tests branch April 22, 2019 07:07
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
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.

2 participants