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

Widget resize tests #3620

Merged
merged 9 commits into from
Mar 28, 2019
Merged

Widget resize tests #3620

merged 9 commits into from
Mar 28, 2019

Conversation

ranbena
Copy link
Contributor

@ranbena ranbena commented Mar 21, 2019

What type of PR is this? (check all applicable)

  • Other - Tests

Description

Screen Shot 2019-03-23 at 15 46 54

@ghost ghost assigned ranbena Mar 21, 2019
@ghost ghost added the in progress label Mar 21, 2019
@ranbena ranbena force-pushed the drag-tests branch 2 times, most recently from db959a9 to 7e4a821 Compare March 23, 2019 13:21
@ranbena ranbena changed the base branch from drag-tests to master March 23, 2019 13:38
@ranbena ranbena changed the base branch from master to drag-tests March 23, 2019 13:40
@ranbena ranbena force-pushed the resize-tests branch 2 times, most recently from d708729 to 689e1a6 Compare March 23, 2019 13:50
gridstack-item="widget.options.position" gridstack-item-id="{{ widget.id }}">
<div class="grid-stack-item-content" data-test="WidgetId{{ widget.id }}">
gridstack-item="widget.options.position" gridstack-item-id="{{ widget.id }}" data-test="WidgetId{{ widget.id }}">
<div class="grid-stack-item-content">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ranbena ranbena requested a review from gabrieldutra March 23, 2019 13:55
@ranbena ranbena marked this pull request as ready for review March 23, 2019 13:55
@ranbena ranbena changed the title Widget resize tests Widget resize tests (wip) Mar 24, 2019
@ranbena
Copy link
Contributor Author

ranbena commented Mar 24, 2019

wip - I'm adding another resize related test

@ranbena ranbena changed the base branch from drag-tests to master March 25, 2019 17:18
@ranbena ranbena changed the title Widget resize tests (wip) Widget resize tests Mar 25, 2019
@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

Added 2 tests for auto height cause resizing disables it.

@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

@gabrieldutra this is ready for review

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.

Not that different from the Drag tests 😁. Left some comments :)

cy.get('@refreshButton').click();
cy.wait('@FreshResults');

// expect height to stay unchanged (would have been 435)
Copy link
Member

Choose a reason for hiding this comment

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

Just to show a different way to assert the same thing:

expect(() => {
  // add 4 table rows
  cy.get('@paramInput').clear().type('5');
  cy.get('@refreshButton').click();
  cy.wait('@FreshResults');
}).not.to.change(this.widget, 'height');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh that's nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I don't think this specifically would work cause

  1. height is a method, not a property.
  2. this.widget is a cy object, not jquery.

Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally the dragBy and resizeBy functions would not return a delta value but instead:

it('moves one column', () => {
  const getLeft = () => $el.offset().left;
  const drag = () => dragBy($el, 200);

  expect(drag).to.increase(getLeft).by(200);
});

Then resizeBy would simply be a dragBy of the element's handle. So nice.

BUT, disconnecting the drag from the delta calculation is currently problematic since the dragged element animates to its new position on mouseup (compensated by calculating the drag placeholder position instead).
Any workaround suggestion? (wait for animation end, disable animation for test, ...)

Copy link
Member

Choose a reason for hiding this comment

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

Really cool, once I get to my laptop I'll run a few tests with it. For the questions:

But I don't think this specifically would work cause

It worked for success, need to try on failure haha. The assertion is essentially the same, they just run it before the function and then guarantee it's the same after.

height is a method, not a property

I hope not to have seen the above wrongly, but this is not supposed to be a problem

this.widget is a cy object, not jquery.

Need to confirm, but I think it is a jQuery object, when I saw it in docs it seemed the same as getting the result of cy.get('@widget')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you can make it work that would be awesome 👍

Copy link
Member

Choose a reason for hiding this comment

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

I could not 😅. That works well with javascript objects, not with elements (I could use a helper object to represent the element, but I don't think it is worth it for my example).

Yours is much better, it would make tests a lot more readable.

Any workaround suggestion?

Can we use the placeholder position for the getLeft method? If not, NP, we can move on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we use the placeholder position for the getLeft method? If not, NP, we can move on.

It doesn't get rendered in the dom until mousedown (or mousemove, I didn't check) so it's not ideal.

@ranbena ranbena merged commit fe4a7b6 into master Mar 28, 2019
@ranbena ranbena deleted the resize-tests branch March 28, 2019 03:55
@ranbena ranbena mentioned this pull request Apr 1, 2019
1 task
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.

3 participants