add test to EventSinkServicesOverviewList component #3806

nemesis09 · 2019-12-18T21:59:06Z

This PR-

adds unit tests to EventSinkServicesOverviewList component

christianvogt · 2019-12-19T04:17:29Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

This is simply asserting that the static markup is returned.
I don't like tests like this because it needs to be kept in sync at all times. Chances are there will never be a breakage in this test unless we explicitly want to change the markup in which case it's a 1:1 change between the markup and the test itself.
cc @rohitkrai03 @andrewballantyne

christianvogt · 2019-12-19T04:19:04Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

How is this a useful test? It's asserting that the wrapper exists. I don't believe there is a way to ever make this fail.

christianvogt · 2019-12-19T04:22:07Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

Can this simply be expect(wrapper.text().includes('No services found for this resource.')).toBe(true)
That way we're not bound to the markup itself.
Not sure it has much value to test that the class text-muted is set because it's not conditional.

christianvogt · 2019-12-19T04:24:03Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

Do wrapper.find(ResourceLink) only once and save it to a variable.

christianvogt · 2019-12-19T04:24:39Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

Missing test case for when there is and isn't a sinkUri present.

nemesis09 · 2019-12-19T22:36:57Z

@christianvogt
I have made changes addressing the reviews. PTAL

nemesis09 · 2019-12-19T22:38:30Z

/retest

nemesis09 · 2019-12-20T12:39:14Z

/retest

invincibleJai · 2019-12-20T16:07:18Z

@nemesis09 there is some issue with the build , frontend-ci is failing https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_console/3806/pull-ci-openshift-console-master-backend/15807

invincibleJai · 2019-12-20T16:10:52Z

frontend/packages/knative-plugin/src/components/overview/__mocks__/overview-knative-mock.ts

Missing newline at end of the file and seems we need to move this in test data, take a look at review docs

__mocks__ folder is for jest mocks https://jestjs.io/docs/en/manual-mocks
Keep test data in the __tests__ dir or re-usable in the src/test/data dir.

Moved the test data to test folder

invincibleJai · 2019-12-20T16:13:04Z

frontend/packages/knative-plugin/src/components/overview/__mocks__/overview-knative-mock.ts

instead of copying mockEventSourceData again we can update test in test block itself with uri

invincibleJai · 2019-12-20T16:15:32Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

description saysResourceLink and expect is checking ExternalLink. it isn't matching

debsmita1 · 2019-12-20T20:18:53Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

both the test cases have the same description but testing different scenarios

invincibleJai · 2019-12-24T08:34:31Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

can you move it inside it block

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

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

build is still failing, try import * as _ from 'lodash-es'; cc @karthikjeeyar

I tried loadash-es but yarn lint flags use of loadash-es as error.

nemesis09 · 2019-12-24T11:03:28Z

/hold

nemesis09 · 2019-12-24T11:06:34Z

/hold cancel

invincibleJai · 2019-12-26T13:33:34Z

...tend/packages/knative-plugin/src/components/overview/__tests__/overview-knative-test-data.ts

can we have type of mockEventSourceData?

nemesis09 · 2019-12-30T09:34:59Z

/test analyze

invincibleJai · 2020-01-02T06:23:53Z

...tend/packages/knative-plugin/src/components/overview/__tests__/overview-knative-test-data.ts

this file i.e test data won't be needed, have refactored it in #3752, Just FYI

I added a test data to this file in a PR for separate component. So I refactored it there. I could remove the file from this PR as well

you can even just undo changes in Routes* components, will be handled in #3752

andrewballantyne · 2020-01-02T19:50:23Z

/kind cleanup

invincibleJai · 2020-01-03T04:52:01Z

@nemesis09 #3752 got merged so you just need to rebase

andrewballantyne

Need another test to properly cover the rendering possibilities of EventSinkServiceOverviewList.

andrewballantyne · 2020-01-05T22:28:02Z

...ages/knative-plugin/src/components/overview/__tests__/EventSinkServicesOverviewList.spec.tsx

Per Christian's comment, need a test to cover the positive path of having a sinkUri.

andrewballantyne · 2020-01-07T14:10:22Z

/test analyze

andrewballantyne · 2020-01-07T15:58:10Z

/test analyze

Bundle size flake...

invincibleJai · 2020-01-08T07:16:25Z

/lgtm

openshift-ci-robot · 2020-01-08T07:16:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [andrewballantyne]
~~frontend/packages/knative-plugin/OWNERS~~ [andrewballantyne,invincibleJai]

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 18, 2019

openshift-ci-robot requested review from invincibleJai and rohitkrai03 December 18, 2019 21:59

openshift-ci-robot added the component/knative Related to knative-plugin label Dec 18, 2019

christianvogt reviewed Dec 19, 2019

View reviewed changes

nemesis09 force-pushed the test-event-source-overview branch from f750b7a to b4114f8 Compare December 19, 2019 22:11

invincibleJai reviewed Dec 20, 2019

View reviewed changes

debsmita1 reviewed Dec 20, 2019

View reviewed changes

nemesis09 force-pushed the test-event-source-overview branch from b4114f8 to 5236b92 Compare December 24, 2019 00:49

invincibleJai reviewed Dec 24, 2019

View reviewed changes

nemesis09 force-pushed the test-event-source-overview branch 2 times, most recently from 36cf0b9 to 2eae165 Compare December 24, 2019 10:34

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 24, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 24, 2019

nemesis09 force-pushed the test-event-source-overview branch 2 times, most recently from 672f8bb to 17b7d4e Compare December 24, 2019 13:28

invincibleJai reviewed Dec 26, 2019

View reviewed changes

nemesis09 force-pushed the test-event-source-overview branch from 17b7d4e to 9e14414 Compare December 27, 2019 10:10

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 27, 2019

nemesis09 force-pushed the test-event-source-overview branch from 9e14414 to f4876ef Compare December 31, 2019 01:05

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

nemesis09 force-pushed the test-event-source-overview branch from f4876ef to 138ff4d Compare December 31, 2019 12:24

invincibleJai reviewed Jan 2, 2020

View reviewed changes

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

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2020

nemesis09 force-pushed the test-event-source-overview branch from 138ff4d to 14e7698 Compare January 3, 2020 23:36

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2020

andrewballantyne suggested changes Jan 5, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 5, 2020

add test to EventSinkServicesOverviewList component

bfc5e82

nemesis09 force-pushed the test-event-source-overview branch from 14e7698 to bfc5e82 Compare January 7, 2020 11:48

andrewballantyne approved these changes Jan 7, 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 Jan 7, 2020

openshift-ci-robot assigned invincibleJai Jan 8, 2020

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

openshift-merge-robot merged commit 777acdf into openshift:master Jan 8, 2020

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

add test to EventSinkServicesOverviewList component #3806

add test to EventSinkServicesOverviewList component #3806

Uh oh!

Conversation

nemesis09 commented Dec 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesis09 commented Dec 19, 2019

Uh oh!

nemesis09 commented Dec 19, 2019

Uh oh!

nemesis09 commented Dec 20, 2019

Uh oh!

invincibleJai commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

invincibleJai Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesis09 commented Dec 24, 2019

Uh oh!

nemesis09 commented Dec 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesis09 commented Dec 30, 2019

Uh oh!

invincibleJai Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewballantyne commented Jan 2, 2020

Uh oh!

invincibleJai commented Jan 3, 2020

Uh oh!

andrewballantyne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

invincibleJai commented Dec 20, 2019 •

edited

Loading

invincibleJai Dec 24, 2019 •

edited

Loading

invincibleJai Jan 2, 2020 •

edited

Loading