Add unit tests for ProjectsExistWrapper #3817

vikram-raj · 2019-12-20T10:15:38Z

Task - https://issues.redhat.com/browse/ODC-2594

divyanshiGupta · 2019-12-20T12:54:57Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+import * as _ from 'lodash';
+import ProjectsExistWrapper from '../ProjectsExistWrapper';
+
+const projects: any = {


Can you create an interface for this object instead of using any?

This should be the project prop using React.ComponentProps of the ProjectsExistsWrapper

rohitkrai03 · 2019-12-20T13:04:16Z

frontend/packages/dev-console/src/components/EmptyState.tsx

  hintBlock = 'Select a way to create an application, component or service from one of the options.',
 }) => {
+  const activeNamespace = getActiveNamespace();


Why are using getActiveNamespace() here which directly hooks into the redux store variable and gives back the result? This is not a correct convention to use redux state and console code does it wrong. We should always use connect to get the values from redux state.

The old code was correct. Please revert changes to this file.

abhi-kn · 2019-12-20T15:30:22Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+        name: 'cvogt',
+        selfLink: '/apis/project.openshift.io/v1/projects/cvogt',


Better if we use some test names & avoid any real usernames

invincibleJai · 2019-12-20T15:58:35Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+    const wrapper = shallow(
+      <ProjectsExistWrapper title="Topology" projects={projects}>
+        {}
+      </ProjectsExistWrapper>,
+    );


nit: we can have this in beforeEach

It's not used exactly the same in every test.

@invincibleJai It is different for each test. That is why I define it for each test.

christianvogt · 2019-12-20T20:17:12Z

frontend/packages/dev-console/src/components/EmptyState.tsx

  hintBlock = 'Select a way to create an application, component or service from one of the options.',
 }) => {
+  const activeNamespace = getActiveNamespace();


The old code was correct. Please revert changes to this file.

christianvogt · 2019-12-20T20:19:48Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+  it('should render projects exist wrapper component', () => {
+    const wrapper = shallow(
+      <ProjectsExistWrapper title="Topology" projects={projects}>
+        {}


children is the required prop for ProjectsExistWrapper.

christianvogt · 2019-12-20T20:23:04Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+    const wrapper = shallow(
+      <ProjectsExistWrapper title="Topology" projects={projects}>
+        {}
+      </ProjectsExistWrapper>,
+    );


It's not used exactly the same in every test.

christianvogt · 2019-12-20T20:24:18Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+        {}
+      </ProjectsExistWrapper>,
+    );
+    expect(wrapper).toBeTruthy();


Not a useful test because I don't think this could ever fail.

christianvogt · 2019-12-20T20:30:07Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+import * as _ from 'lodash';
+import ProjectsExistWrapper from '../ProjectsExistWrapper';
+
+const projects: any = {


This should be the project prop using React.ComponentProps of the ProjectsExistsWrapper

christianvogt · 2019-12-20T20:32:01Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+        {}
+      </ProjectsExistWrapper>,
+    );
+    expect(wrapper.find('LoadingBox').exists()).toBeTruthy();


Use the component constructor and not a string.
find(LoadingBox)

christianvogt · 2019-12-20T20:32:57Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+      </ProjectsExistWrapper>,
+    );
+    const hintBlock = shallow(wrapper.props().hintBlock);
+    expect(wrapper.find('ODCEmptyState').exists()).toBeTruthy();


This test should end on this line. The rest is testing static markup.

christianvogt · 2019-12-20T20:33:28Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+    ];
+    const wrapper = shallow(
+      <ProjectsExistWrapper title="Topology" projects={data}>
+        {() => {


Use a constant here and in the assertion below.

christianvogt · 2019-12-20T20:34:24Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+        }}
+      </ProjectsExistWrapper>,
+    );
+    expect(wrapper.contains(<span>Child component</span>)).toBeTruthy();


This should be more specific and assert that the only contents is the given children.

abhi-kn · 2019-12-23T09:46:57Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+  });
+
+  it('should show loading box', () => {
+    const data = _.cloneDeep(projects);


It might be better to define default project props in beforeEach and have necessary overrides in each test case.

christianvogt · 2019-12-23T18:48:24Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+    projects.loaded = false;
+    const wrapper = shallow(
+      <ProjectsExistWrapper title="Topology" projects={projects}>
+        {}


Why are you rendering an empty block of children instead of using a self closing tag? What's the point of {}

I fixed it. I was thinking of something else.

christianvogt · 2019-12-23T18:50:13Z

frontend/packages/dev-console/src/components/__tests__/ProjectsExistWrapper.spec.tsx

+        {}
+      </ProjectsExistWrapper>,
+    );
+    const hintBlock = shallow(wrapper.props().hintBlock);


This is not a useful test as you are testing another component other than the ProjectExistsWrapper and it is also not a conditional prop value.

I just wanted to test the empty state here. If we have no projects then it should render the empty state.

If there are no projects assert presence of ODCEmptyState.

Avoid asserting the markup of a component not owned by the component you are testing. Your unit test becomes dependent on the implementation details of another component which is bad.

Fixed this. Now, checking for only ODCEmptyState is present if no project it present.

invincibleJai · 2019-12-24T08:47:07Z

/lgtm

invincibleJai · 2020-01-02T06:50:40Z

/lgtm

andrewballantyne · 2020-01-02T19:43:33Z

/kind cleanup

andrewballantyne

/assign
/retest

openshift-ci-robot · 2020-01-06T02:41:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, invincibleJai, vikram-raj

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/dev-console/OWNERS~~ [andrewballantyne]

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

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 20, 2019

openshift-ci-robot requested review from abhi-kn and christianvogt December 20, 2019 10:15

openshift-ci-robot added the component/dev-console Related to dev-console label Dec 20, 2019

vikram-raj force-pushed the odc-2594 branch from 8cf732e to 6b176ea Compare December 20, 2019 10:16

vikram-raj changed the title ~~Add unit test for ProjectsExistWrapper~~ Add unit tests for ProjectsExistWrapper Dec 20, 2019

vikram-raj force-pushed the odc-2594 branch from 6b176ea to 074c116 Compare December 20, 2019 10:43

divyanshiGupta reviewed Dec 20, 2019

View reviewed changes

rohitkrai03 suggested changes Dec 20, 2019

View reviewed changes

abhi-kn reviewed Dec 20, 2019

View reviewed changes

invincibleJai reviewed Dec 20, 2019

View reviewed changes

christianvogt reviewed Dec 20, 2019

View reviewed changes

abhi-kn reviewed Dec 23, 2019

View reviewed changes

vikram-raj force-pushed the odc-2594 branch from 074c116 to c7ae7f5 Compare December 23, 2019 10:02

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Dec 23, 2019

vikram-raj force-pushed the odc-2594 branch from c7ae7f5 to 1c19cc1 Compare December 23, 2019 10:09

christianvogt reviewed Dec 23, 2019

View reviewed changes

vikram-raj force-pushed the odc-2594 branch from 1c19cc1 to ddf962d Compare December 23, 2019 18:57

vikram-raj requested a review from christianvogt December 23, 2019 19:00

openshift-ci-robot assigned invincibleJai Dec 24, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 24, 2019

Add unit tests for ProjectsExistWrapper

ba7db61

vikram-raj force-pushed the odc-2594 branch from ddf962d to ba7db61 Compare December 26, 2019 06:13

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 26, 2019

vikram-raj requested a review from invincibleJai January 2, 2020 06:31

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

openshift-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 2, 2020

andrewballantyne approved these changes Jan 6, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 6, 2020

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

openshift-merge-robot merged commit 91fb810 into openshift:master Jan 6, 2020

spadgett added this to the v4.4 milestone Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for ProjectsExistWrapper #3817

Add unit tests for ProjectsExistWrapper #3817

vikram-raj commented Dec 20, 2019

divyanshiGupta Dec 20, 2019

christianvogt Dec 20, 2019

rohitkrai03 Dec 20, 2019

christianvogt Dec 20, 2019

abhi-kn Dec 20, 2019

invincibleJai Dec 20, 2019

christianvogt Dec 20, 2019

vikram-raj Dec 23, 2019 •

edited

christianvogt Dec 20, 2019

christianvogt Dec 20, 2019

vikram-raj Dec 23, 2019

christianvogt Dec 20, 2019

christianvogt Dec 20, 2019

christianvogt Dec 20, 2019

christianvogt Dec 20, 2019

christianvogt Dec 20, 2019

christianvogt Dec 20, 2019

christianvogt Dec 20, 2019

abhi-kn Dec 23, 2019

christianvogt Dec 23, 2019

vikram-raj Dec 23, 2019

christianvogt Dec 23, 2019

vikram-raj Dec 23, 2019

christianvogt Dec 25, 2019

vikram-raj Dec 26, 2019

invincibleJai commented Dec 24, 2019

invincibleJai commented Jan 2, 2020

andrewballantyne commented Jan 2, 2020

andrewballantyne left a comment

openshift-ci-robot commented Jan 6, 2020

		name: 'cvogt',
		selfLink: '/apis/project.openshift.io/v1/projects/cvogt',

Add unit tests for ProjectsExistWrapper #3817

Add unit tests for ProjectsExistWrapper #3817

Conversation

vikram-raj commented Dec 20, 2019

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

vikram-raj Dec 23, 2019 • 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

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

invincibleJai commented Dec 24, 2019

invincibleJai commented Jan 2, 2020

andrewballantyne commented Jan 2, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 6, 2020

vikram-raj Dec 23, 2019 •

edited