Bug 1868475: Adds test for independent Mode Dashboard #5293

a2batic · 2020-05-04T23:48:56Z

Unit test on breakdown card and details card for independent mode.

Signed-off-by: Kanika kmurarka@redhat.com

gnehapk · 2020-05-12T06:02:25Z

frontend/packages/ceph-storage-plugin/src/__tests__/independent-mode/breakdown-card.spec.tsx

+import { BreakdownCardBody } from "../../components/dashboard-page/storage-dashboard/breakdown-card/breakdown-body";
+import DashboardCardTitle from "@console/shared/src/components/dashboard/dashboard-card/DashboardCardTitle";
+import { HeaderPrometheusViewLink } from "../../components/dashboard-page/storage-dashboard/breakdown-card/breakdown-header";
+


Sort the imports properly

a2batic · 2020-05-27T06:17:48Z

/assign @vojtechszocs

a2batic · 2020-05-27T06:18:00Z

/test e2e-gcp-console

bipuladh

I do not understand the gist of these tests. If these are tests for Individual cards then we should move them to specific cards. I am not sure if we are testing specifically anything in Independent Mode dashboard.

bipuladh · 2020-05-28T04:04:14Z

frontend/packages/ceph-storage-plugin/src/__tests__/independent-mode/breakdown-card.spec.tsx

+  it('Should render Card Header', () => {
+    expect(wrapper.find(DashboardCardHeader).exists()).toBe(true);
+  });
+
+  it('Should render Card Title', () => {
+    expect(wrapper.find(DashboardCardTitle).exists()).toBe(true);
+  });
+
+  it('Should render Prometheus Header Link', () => {
+    expect(wrapper.find(HeaderPrometheusViewLink).exists()).toBe(true);
+    expect(wrapper.find(HeaderPrometheusViewLink).props().link).toEqual(headerLink);
+  });
+
+  it('Should render Dropdown', () => {
+    expect(wrapper.find(Dropdown).exists()).toBe(true);
+    expect(wrapper.find(Dropdown).props().items).toEqual(dropDownItems);
+    expect(wrapper.find(Dropdown).props().title).toEqual('Projects');
+  });
+
+  it('Should render Card body', () => {
+    expect(wrapper.find(DashboardCardBody).exists()).toBe(true);
+  });
+
+  it('Should render Breakdown Card body', () => {
+    expect(wrapper.find(BreakdownCardBody).exists()).toBe(true);
+    expect(wrapper.find(BreakdownCardBody).props().isLoading).toBe(true);
+    expect(wrapper.find(BreakdownCardBody).props().hasLoadError).toBe(false);
+  });
+});


These tests are part of Breakdown Card right? Why are we testing them again? We should only test whether Breakdown card is getting rendered or not. If these tests are not there in Breakdown lets add them there rather than here.

@bipuladh these tests are for breakdown card of independent mode. All these component are part of the card directly, looking at the component.

I believe test related to how breakdown card uses props should be part of the breakdown-card test. But, as part of this card, we should add test related to expected props been sent to breakdown component, because they are set and calculated here.

These tests are part of Breakdown Card right?

It seems that BreakdownCard component tested here is OCS / independent mode specific. The code is currently in src/components/independent-dashboard-page/breakdown-card/card.tsx.

Similarly, there is e.g. (OCS specific) UtilizationCard in src/components/independent-dashboard-page/utilization-card/card.tsx.

Both of these are built on top of DashboardCard which is a core Console component.

What we're missing here (I think) is a test for the core DashboardCard component:

git grep 'DashboardCard' *.spec.* # no results

For now, I'm OK with these tests being here. In future, we should refactor & move relevant bits into ^^ spec.

cc @rawagner

Once we have a core DashboardCard spec, tests for OCS specific components built on top of it should only test the OCS specifics (in terms of their render output or behavior).

bipuladh · 2020-05-28T04:05:37Z

frontend/packages/ceph-storage-plugin/src/__tests__/independent-mode/details-card.spec.tsx

+  it('Should render Card Header', () => {
+    expect(wrapper.find(DashboardCardHeader).exists()).toBe(true);
+  });
+
+  it('Should render Card Title', () => {
+    expect(wrapper.find(DashboardCardTitle).exists()).toBe(true);
+  });
+
+  it('Should render details properly', () => {
+    expect(wrapper.find('[data-test-id="cluster-name"]').text()).toEqual('foo');
+    expect(wrapper.find('[data-test-id="cluster-subscription"]').text()).toEqual('fooVersion');
+  });
+});


This again should be part of details card. We can add these tests in console-shared.

vojtechszocs · 2020-06-03T16:44:25Z

frontend/packages/ceph-storage-plugin/src/__mocks__/independent-mode-dashboards.ts

+  urlResults: 'foo',
+  prometheusResults: {
+    // eslint-disable-next-line no-new-func
+    getIn: new Function(),


We can simply use the function declaration syntax:

Suggested change

getIn: new Function(),

getIn: () => {},

Using the new Function constructor is generally deprecated since it has the following characteristics:

it is completely dynamic, i.e. function's body is a string that needs to be parsed & evaluated (*)

runs in global context, i.e. the code does not create closures to their creation context (unlike eval)

(*) this also has security & performance implications

vojtechszocs · 2020-06-03T16:55:26Z

frontend/packages/ceph-storage-plugin/src/__mocks__/independent-mode-dashboards.ts

@@ -0,0 +1,67 @@
+export const DashboardItems = {


I'd suggest naming this file independent-mode-dashboard-data.ts since it contains test data.

Also, I'd name this dashboardData

Suggested change

export const DashboardItems = {

export const dashboardData = {

vojtechszocs · 2020-06-03T17:05:20Z

frontend/packages/ceph-storage-plugin/src/__tests__/independent-mode/breakdown-card.spec.tsx

@@ -0,0 +1,68 @@
+import * as React from 'react';


Why do we have a structure like:

$ tree packages/ceph-storage-plugin/src/components/independent-dashboard-page packages/ceph-storage-plugin/src/components/independent-dashboard-page |-- breakdown-card | `-- card.tsx |-- details-card | `-- card.tsx |-- status-card | `-- card.tsx `-- utilization-card `-- card.tsx

I'd suggest to simplify it to:

|-- breakdown-card.tsx |-- details-card.tsx |-- status-card.tsx `-- utilization-card.tsx

Also, please keep the path convention for better test vs. tested-unit correlation, by placing this spec at

src/__tests__/independent-dashboard-page/breakdown-card.spec.tsx

I see that you're following a single-test-directory convention in Ceph plugin:

$ find packages/ceph-storage-plugin/src -type d -name __tests__ packages/ceph-storage-plugin/src/__tests__

in which case it's more important to keep your __tests__ structure well organized, in my opinion.

vojtechszocs · 2020-06-03T17:20:39Z

frontend/packages/ceph-storage-plugin/src/__tests__/independent-mode/breakdown-card.spec.tsx

+  beforeEach(() => {
+    wrapper = shallow(
+      <BreakdownCard
+        watchPrometheus={DashboardItems.watchPrometheus as any}


Suggested change

watchPrometheus={DashboardItems.watchPrometheus as any}

watchPrometheus={dashboardData.watchPrometheus as any}

vojtechszocs · 2020-06-03T17:24:31Z

frontend/packages/ceph-storage-plugin/src/__mocks__/independent-mode-dashboards.ts

+  'Storage Classes': 'Storage Classes',
+};
+
+export const headerLink =


headerLink and dropDownItems are treated as expected test data.

Therefore, I'd suggest naming them expectedDropDownItems and expectedHeaderLink respectively.

vojtechszocs · 2020-06-03T17:27:28Z

frontend/packages/ceph-storage-plugin/src/__mocks__/independent-mode-dashboards.ts

+export const headerLink =
+  'topk(20, (sum(kubelet_volume_stats_used_bytes * on (namespace,persistentvolumeclaim) group_left(storageclass, provisioner) (kube_persistentvolumeclaim_info * on (storageclass)  group_left(provisioner) kube_storageclass_info {provisioner=~"(.*rbd.csi.ceph.com)|(.*cephfs.csi.ceph.com)|(ceph.rook.io/block)"})) by (namespace)))';
+
+export const detailsCardData = {


Why not simply move detailsCardData.resources into dashboardData.resources and remove the detailsCardData export?

This way, you'll have a single source of test data (dashboardData) that can be used in all relevant tests.

vojtechszocs · 2020-06-03T17:38:25Z

frontend/packages/ceph-storage-plugin/src/__tests__/independent-mode/breakdown-card.spec.tsx

+  it('Should render Card Header', () => {
+    expect(wrapper.find(DashboardCardHeader).exists()).toBe(true);
+  });
+
+  it('Should render Card Title', () => {
+    expect(wrapper.find(DashboardCardTitle).exists()).toBe(true);
+  });
+
+  it('Should render Prometheus Header Link', () => {
+    expect(wrapper.find(HeaderPrometheusViewLink).exists()).toBe(true);
+    expect(wrapper.find(HeaderPrometheusViewLink).props().link).toEqual(headerLink);
+  });
+
+  it('Should render Dropdown', () => {
+    expect(wrapper.find(Dropdown).exists()).toBe(true);
+    expect(wrapper.find(Dropdown).props().items).toEqual(dropDownItems);
+    expect(wrapper.find(Dropdown).props().title).toEqual('Projects');
+  });
+
+  it('Should render Card body', () => {
+    expect(wrapper.find(DashboardCardBody).exists()).toBe(true);
+  });
+
+  it('Should render Breakdown Card body', () => {
+    expect(wrapper.find(BreakdownCardBody).exists()).toBe(true);
+    expect(wrapper.find(BreakdownCardBody).props().isLoading).toBe(true);
+    expect(wrapper.find(BreakdownCardBody).props().hasLoadError).toBe(false);
+  });
+});


These tests are part of Breakdown Card right?

It seems that BreakdownCard component tested here is OCS / independent mode specific. The code is currently in src/components/independent-dashboard-page/breakdown-card/card.tsx.

Similarly, there is e.g. (OCS specific) UtilizationCard in src/components/independent-dashboard-page/utilization-card/card.tsx.

Both of these are built on top of DashboardCard which is a core Console component.

What we're missing here (I think) is a test for the core DashboardCard component:

git grep 'DashboardCard' *.spec.* # no results

For now, I'm OK with these tests being here. In future, we should refactor & move relevant bits into ^^ spec.

cc @rawagner

vojtechszocs · 2020-06-03T17:42:21Z

frontend/packages/ceph-storage-plugin/src/__tests__/independent-mode/breakdown-card.spec.tsx

+  it('Should render Card Header', () => {
+    expect(wrapper.find(DashboardCardHeader).exists()).toBe(true);
+  });
+
+  it('Should render Card Title', () => {
+    expect(wrapper.find(DashboardCardTitle).exists()).toBe(true);
+  });
+
+  it('Should render Prometheus Header Link', () => {
+    expect(wrapper.find(HeaderPrometheusViewLink).exists()).toBe(true);
+    expect(wrapper.find(HeaderPrometheusViewLink).props().link).toEqual(headerLink);
+  });
+
+  it('Should render Dropdown', () => {
+    expect(wrapper.find(Dropdown).exists()).toBe(true);
+    expect(wrapper.find(Dropdown).props().items).toEqual(dropDownItems);
+    expect(wrapper.find(Dropdown).props().title).toEqual('Projects');
+  });
+
+  it('Should render Card body', () => {
+    expect(wrapper.find(DashboardCardBody).exists()).toBe(true);
+  });
+
+  it('Should render Breakdown Card body', () => {
+    expect(wrapper.find(BreakdownCardBody).exists()).toBe(true);
+    expect(wrapper.find(BreakdownCardBody).props().isLoading).toBe(true);
+    expect(wrapper.find(BreakdownCardBody).props().hasLoadError).toBe(false);
+  });
+});


Once we have a core DashboardCard spec, tests for OCS specific components built on top of it should only test the OCS specifics (in terms of their render output or behavior).

a2batic · 2020-06-29T09:44:40Z

@vojtechszocs addressed review comments, please review.

Signed-off-by: Kanika <kmurarka@redhat.com>

a2batic · 2020-08-12T16:56:43Z

@vojtechszocs addressed review comments, please review.

a2batic · 2020-08-12T16:56:56Z

/test e2e-gcp-console

openshift-ci-robot · 2020-08-12T19:14:59Z

@a2batic: This pull request references Bugzilla bug 1868475, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1868475: Adds test for independent Mode Dashboard

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

a2batic · 2020-08-12T19:15:34Z

/bugzilla refresh

openshift-ci-robot · 2020-08-12T19:15:41Z

@a2batic: This pull request references Bugzilla bug 1868475, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

a2batic · 2020-08-12T19:19:59Z

/test e2e-gcp-console

vojtechszocs · 2020-08-13T12:55:34Z

/lgtm

openshift-ci-robot · 2020-08-13T12:55:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, gnehapk, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/ceph-storage-plugin/OWNERS~~ [gnehapk,vojtechszocs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-08-13T12:57:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-13T17:17:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-13T19:27:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-13T19:40:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

a2batic · 2020-08-14T07:06:50Z

/retest

openshift-ci-robot · 2020-08-14T11:09:34Z

@a2batic: All pull requests linked via external trackers have merged: openshift/console#5293. Bugzilla bug 1868475 has been moved to the MODIFIED state.

In response to this:

Bug 1868475: Adds test for independent Mode Dashboard

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/ceph Related to ceph-storage-plugin labels May 4, 2020

openshift-ci-robot requested review from rawagner and shyRozen May 4, 2020 23:49

gnehapk reviewed May 12, 2020

View reviewed changes

a2batic force-pushed the indy-dash-test branch from b74ad76 to abc7533 Compare May 26, 2020 20:56

a2batic changed the title ~~[WIP] Adds test for independent Mode Dashboard~~ Adds test for independent Mode Dashboard May 26, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2020

openshift-ci-robot assigned vojtechszocs May 27, 2020

gnehapk approved these changes May 28, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2020

bipuladh reviewed May 28, 2020

View reviewed changes

vojtechszocs suggested changes Jun 3, 2020

View reviewed changes

a2batic force-pushed the indy-dash-test branch from abc7533 to 84eeb16 Compare June 16, 2020 06:27

a2batic requested review from vojtechszocs and bipuladh June 16, 2020 09:12

Adds test for independent Mode Dashboard

703c7fa

Signed-off-by: Kanika <kmurarka@redhat.com>

a2batic force-pushed the indy-dash-test branch from 84eeb16 to 703c7fa Compare August 12, 2020 13:40

a2batic changed the title ~~Adds test for independent Mode Dashboard~~ Bug 1868475 : Adds test for independent Mode Dashboard Aug 12, 2020

a2batic changed the title ~~Bug 1868475 : Adds test for independent Mode Dashboard~~ Bug 1868475: Adds test for independent Mode Dashboard Aug 12, 2020

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Aug 12, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Aug 12, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 12, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Aug 12, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2020

openshift-merge-robot merged commit 950853a into openshift:master Aug 14, 2020

spadgett added this to the v4.6 milestone Aug 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1868475: Adds test for independent Mode Dashboard #5293

Bug 1868475: Adds test for independent Mode Dashboard #5293

a2batic commented May 4, 2020 •

edited

gnehapk May 12, 2020

a2batic commented May 27, 2020

a2batic commented May 27, 2020

bipuladh left a comment

bipuladh May 28, 2020

a2batic May 28, 2020 •

edited

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

bipuladh May 28, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

vojtechszocs Jun 3, 2020

a2batic commented Jun 29, 2020

a2batic commented Aug 12, 2020

a2batic commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

a2batic commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

a2batic commented Aug 12, 2020

vojtechszocs commented Aug 13, 2020

openshift-ci-robot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

a2batic commented Aug 14, 2020

openshift-ci-robot commented Aug 14, 2020

	export const DashboardItems = {
	export const dashboardData = {

	watchPrometheus={DashboardItems.watchPrometheus as any}
	watchPrometheus={dashboardData.watchPrometheus as any}

Bug 1868475: Adds test for independent Mode Dashboard #5293

Bug 1868475: Adds test for independent Mode Dashboard #5293

Conversation

a2batic commented May 4, 2020 • edited

Choose a reason for hiding this comment

a2batic commented May 27, 2020

a2batic commented May 27, 2020

bipuladh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2batic May 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2batic commented Jun 29, 2020

a2batic commented Aug 12, 2020

a2batic commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

a2batic commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

a2batic commented Aug 12, 2020

vojtechszocs commented Aug 13, 2020

openshift-ci-robot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

a2batic commented Aug 14, 2020

openshift-ci-robot commented Aug 14, 2020

a2batic commented May 4, 2020 •

edited

a2batic May 28, 2020 •

edited