Widget tests - add, remove, auto height

[ranbena]

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

• Other - Tests

Description

Added Cypress tests for widgets. Creates query on the fly and tests:

1. Adds widget.
2. Removes widget.
3. Auto height feature test.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[ranbena]

@gabrieldutra this is ready for your scrutiny 🙇

A few notes:

1. I have MANY more widget tests planned (dimensions, resizing, dragging, responsiveness, etc.)
2. Now that I add a query - Percy 😁
3. After we move the cypress folder we should create some lint overrides.

[ranbena]

This is unnecessary, right? 🤔

[gabrieldutra]

I think the cost for it now isn't worth it (currently 2 sec). I wonder how long it would be if you created the widget by API.

[ranbena]

The problem with creating an API call for Widget/Textbox is that it requires some dynamically generated position data to be sent with it:

data = {
  ...
  options: {
    ...
    position: {
      autoHeight: true,
      col: 3,
      maxSizeX: 6,
      maxSizeY: 1000,
      minSizeX: 1,
      minSizeY: 1,
      row: 0,
      sizeX: 3,
      sizeY: 3,
    },
  },
}

that I'd rather not have to mimic. We could get away with it if the dashboard is a clean slate (cause then it's always the same), though that has a cost of its own.

[gabrieldutra]

The easiest way I see to get a clean state for the dashboard is to create a new one for each test using the dashboard api 😅. I tested timing by removing the before and changing the beforeEach to be this:

describe('Widget', () => {
  beforeEach(function () {
    createNewDashboardByAPI('Foo Bar')
      .then(slug => `/dashboard/${slug}`)
      .as('dashboardUrl')
      .then(cy.visit);
  });
  ...
});

I actually didn't notice any timing increase, the only con I see is that we'll have plenty of dashboards after tests run 😂. Anyway, if you think we have any gain by actually using UI to do it or if the widget api is too tricky to handle, I say we can leave this as is and later see a better option.

[kravets-levko]

@ranbena (cc @gabrieldutra) Actually not all that fields are required (some of them are saved accidentally, one day I'm going to fix that):

position: {
  autoHeight: true,
  col: 3,
  row: 0,
  sizeX: 3,
  sizeY: 3,
},

All that values could be set manually, just check constraints for selected widget type (e.g. not all widgets can have autoHeight=true, some have min width/height; position can be arbitrary at all, just keep in mind that widgets "flow" to top if there is free space)

[ranbena]

Ok I'll go for it.

[ranbena]

Implemented it. Hope the diff isn't too heavy to review..
Here's my summary:
Pros

• -10 seconds run time

Cons (imho):

• Since the ui doesn't update after the "add widget" api request was executed , must reload the page for the widget to appear. Had to move the cy.visit(this.dashboardUrl) from beforeEach to inside the test.
• For the same reason, addTextboxByAPI and addWidgetByAPI can't return an element reference to be aliased, since the element doesn't yet exist in the dom. I tried to work around that limitation but eventually fell back to returning elTestId which will then require the test to cy.getByTestId(elTestId).
• Currently, settling on hardcoded "first widget" positioning config { col: 0, row: 0, sizeX: 3, sizeY: 3 } but if future test requires multiple widgets, must do calculation (or opt for widget add from ui).

Overall I think this added more "magic" to the tests which I favor for production code and less for testing code as readability and scalability are more important than line count and abstraction.

Lmk what you think @gabrieldutra and @LevkO and if you can improve the code.

[gabrieldutra]

Left some small comments, besides that, two things:

• No way to create Widgets by API 😕? I bet this would improve a lot performance here, mainly for the auto height tests, but you did mention an issue for Textboxes;
• Do the auto height tests depend on each other? I mean, if you run blocks separately with it.only('...') (really cool Cypress trick btw) will they continue to pass?

[gabrieldutra]

I think the cost for it now isn't worth it (currently 2 sec). I wonder how long it would be if you created the widget by API.

[ranbena]

I think the cost for it now isn't worth it (currently 2 sec). I wonder how long it would be if you created the widget by API.

@gabrieldutra thing is that these autoHeight checks should be done against a widget with an autoHeight feature, therefore I check these parameters as part of the tests.

Without these checks, I would be assuming that:

• a new query returns with a table vis
• a table vis is configured to have { autoHeight: true }

Which if one day changed, would break the test illegitimately.
Wdyt?

[gabrieldutra]

Which if one day changed, would break the test illegitimately.
Wdyt?

I see, this is a test for the test 😆. The "problem" with it is that you're only asserting this for that specific case of addWidget. I think in this case it won't be an issue, but just to show Cypress cool possibilities:

describe('Auto height for table visualization', () => {
  const haveCorrectParams = ({options, visualization}) => {
    expect(visualization.type).to.eq('TABLE');
    expect(options.position.autoHeight).to.be.true;
  };

  it('renders correct height for 2 table rows', () => {
    const queryData = {
      query: 'select s.a FROM generate_series(1,2) AS s(a)',
    };

    addWidget(queryData)
      .should(haveCorrectParams) // use whenever you want to guarantee the parameters
      .then(({ id }) => {
        cy.getByTestId(`WidgetId${id}`)
          .its('0.offsetHeight')
          .should('eq', 235);
      });
  });
});

I'm not sure I liked this way of coding, but it's a possibility indeed.

[ranbena]

I'm not sure I liked this way of coding, but it's a possibility indeed.

I like it better than my way, but I decided to ditch it entirely for sake of simplicity.

[gabrieldutra]

Just one small comment, the result actually looks fine to me by now, but I'll give my view on your concerns.

Since the ui doesn't update after the "add widget" api request was executed , must reload the page for the widget to appear. Had to move the cy.visit(this.dashboardUrl) from beforeEach to inside the test.

To be honest I don't see a nice fix to this, the options that are in my mind would just change coding style and create another problem somewhere else:

• We could use context's to have shared code:

  context('dashboard with one widget', function () {
    beforeEach(function () {
      addWidgetByAPI(this.dashboardId)
        .as('elTestId')
        .then(cy.visit(this.dashboardUrl));
      cy.getByTestId(elTestId).as('widget');
    });
    // ...
  });

  Problem: You need addWidgetsByAPI() to be flexible for most of future tests, so this would not help that much.

• Create all dashboards you need with their dependencies for the tests prior to everything and then just use them. The problem with this is that the before hook would grow with the number of tests you have.

• Separate those tests into different files (i.e: widget_spec.js, or even widget_auto_height_spec.js). So far I didn't see a need for this, but this could help to organize shared code for each part you're testing.

For the same reason, addTextboxByAPI and addWidgetByAPI can't return an element reference to be aliased, since the element doesn't yet exist in the dom. I tried to work around that limitation but eventually fell back to returning elTestId which will then require the test to cy.getByTestId(elTestId).

I think it's fine to query the element when we need it during tests, it's not that much of a difference anyway 😆

Currently, settling on hardcoded "first widget" positioning config { col: 0, row: 0, sizeX: 3, sizeY: 3 } but if future test requires multiple widgets, must do calculation (or opt for widget add from ui).

I also don't like the calculation idea, but the problem I aimed here was exactly thinking about the context of multiple widgets, with a different perspective though: with multiple widgets time would increase a lot...

I remember seeing more of Cypress creating widgets than actually testing the features when watching it running 😅

All-in-all I think this is good as is, but LMK what you think 👍

[ranbena]

@gabrieldutra thanks for the detailed feedback. Seems we can abstain from any changes to the code and just land this. Can you "approve"?

[gabrieldutra]

Seems we can abstain from any changes to the code and just land this. Can you "approve"?

👍