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

Dashboard one column mode test #3621

Merged
merged 13 commits into from
Apr 8, 2019
Merged

Dashboard one column mode test #3621

merged 13 commits into from
Apr 8, 2019

Conversation

ranbena
Copy link
Contributor

@ranbena ranbena commented Mar 21, 2019

What type of PR is this?

  • Other - Test

Description

Screen Shot 2019-03-24 at 8 40 01

@ghost ghost assigned ranbena Mar 21, 2019
@ghost ghost added the in progress label Mar 21, 2019
@ranbena ranbena requested a review from arikfr March 21, 2019 20:01
@ranbena
Copy link
Contributor Author

ranbena commented Mar 21, 2019

@arikfr any other aspect to test with this feature?

@ranbena ranbena changed the title Dashboard one column mode test Dashboard one column mode test (wip) Mar 23, 2019
@ranbena
Copy link
Contributor Author

ranbena commented Mar 23, 2019

I'll add some controls hide/show testing

@ranbena ranbena changed the title Dashboard one column mode test (wip) Dashboard one column mode test Mar 24, 2019
@ranbena ranbena requested review from gabrieldutra and removed request for arikfr March 24, 2019 06:41
@ranbena ranbena force-pushed the single-col-tests branch 4 times, most recently from 18722ee to 326654d Compare March 26, 2019 09:16
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.

More tests 🙂.

I left some comments, requesting changes more to the ones with assertions that don't work ^^

client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
@ranbena ranbena force-pushed the single-col-tests branch 2 times, most recently from b96ebe6 to 104783e Compare March 29, 2019 19:44
@@ -629,4 +629,69 @@ describe('Dashboard', () => {
});
});
});

context('viewport width is at 800px', () => {
Copy link
Member

@gabrieldutra gabrieldutra Mar 30, 2019

Choose a reason for hiding this comment

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

With context how about moving cy.viewport(800, 800); to its beforeEach hook?

You'll have to invert tests order so the assertion for one column mode comes first, but it seems to make sense:

context('viewport width is at 800px', () => {
  beforeEach(function () {
    cy.visit(this.dashboardUrl);
    cy.viewport(800, 800);
  });

  it('shows widgets with full width', () => {
    cy.get('@textboxEl').should(($el) => {
      expect($el.width()).to.eq(785);
    });
    cy.viewport(801, 800);
    cy.get('@textboxEl').should(($el) => {
      expect($el.width()).to.eq(393);
    });
  });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabrieldutra I went for this, wdyt?

describe('Viewport width affects presentation', () => {
  context('viewport width is at 800px', () => {
    it('shows widgets with full width' ...
    it('hides edit option' ...
    it('disables edit mode' ...
  });

  context('viewport width is above 800px', () => {
    it('shows widgets with specified width' ...
    it('shows edit option' ...
    it('enables edit mode' ...
  });

  context('viewport width is at 767px', () => {
    it('hides menu button' ...
  });

  context('viewport width is above 767px', () => {
     it('shows menu button' ...
  });
});

Copy link
Member

Choose a reason for hiding this comment

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

I would say this would be the state-of-the-art when considering tests organization. However, Cypress has a cost associated to each new it block, so it's worth it to check how much will be added after this.

From this point whatever you want is good to me, so LMK when I can check it again 🙂

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 went for sth leaner:

Screen Shot 2019-04-07 at 18 48 11

@kravets-levko
Copy link
Collaborator

@ranbena Just a note about these tests: does it make any sense to split dashboard_spec.js into smaller files? There are actually two reasons why I think about this:

  1. it's already quite large - with all issues that come from that (harder understanding, navigation in code, etc.).
  2. I use a VM for development with 8GB RAM, and to run this file from Cypress GUI I have to close everything - IDE, browsers, etc. - because this suite needs all free memory to run successfully (I think it's because of Cypress "time machine", but not sure). I know that 8GB is probably not a lot and I should add some memory to VM, but 1) I cannot do it right now for a some reason; 2) it will not solve the issue in long perspective.

WDYT?

@ranbena
Copy link
Contributor Author

ranbena commented Mar 31, 2019

does it make any sense to split dashboard_spec.js into smaller files?

@kravets-levko yes, now that tests are done (for now 😅) I'll split it into 3 files, in a followup PR.

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.

I use a VM for development with 8GB RAM... I should add some memory to VM

It's sort of fun to see you say it, because 8GB RAM is all I've got to my laptop now 😆. Eventually I'm gonna upgrade it, but I didn't see a need so far. I'm not sure how you managed to have issues with it, but I can usually run Cypress tests with 4 GB available (the rest already used by Chrome and other apps).

I agree we should split it anyway in a following PR.

@@ -629,4 +629,69 @@ describe('Dashboard', () => {
});
});
});

context('viewport width is at 800px', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I would say this would be the state-of-the-art when considering tests organization. However, Cypress has a cost associated to each new it block, so it's worth it to check how much will be added after this.

From this point whatever you want is good to me, so LMK when I can check it again 🙂

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.

Looks nice! ☕

client/cypress/integration/dashboard/dashboard_spec.js Outdated Show resolved Hide resolved
@ranbena ranbena merged commit fc5a624 into master Apr 8, 2019
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
@guidopetri guidopetri deleted the single-col-tests branch July 22, 2023 03:18
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