Bug 1907888: Fix pipeline list page loader #7547

karthikjeeyar · 2020-12-15T07:38:39Z

Fixes: https://issues.redhat.com/browse/ODC-5264

Analysis / Root cause:

"Pipelines" list page shows a message "No pipelines found" before it loads the data

Solution Description:

Do not render the No pipelines found text before the firehose call loads the data.

Screenshots:

Unit test coverage report:

Browser conformance:

Chrome
Firefox
Safari
Edge

karthikjeeyar · 2020-12-15T13:07:44Z

/kind bug

openshift-ci-robot · 2020-12-15T13:09:49Z

@karthikjeeyar: This pull request references Bugzilla bug 1907888, 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.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1907888: Fix pipeline list page loader

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.

andrewballantyne · 2020-12-15T15:33:57Z

/retest
/assign

Looks good on a high level, makes sense that's why it's happening. Want to investigate the code a bit more before I approve.

openshift-ci-robot · 2020-12-16T09:23:21Z

@karthikjeeyar: This pull request references Bugzilla bug 1907888, which is valid.

3 validation(s) were run on this bug

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

In response to this:

Bug 1907888: Fix pipeline list page loader

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.

karthikjeeyar · 2020-12-16T10:46:52Z

/retest

andrewballantyne

Sorry, got to this now, not sure what I missed from my first look but there are some comments on your added tests and I think I need to clarify to the team when null vs loading is needed.

andrewballantyne · 2020-12-16T19:02:31Z

.../packages/pipelines-plugin/src/components/pipelines/list-page/PipelineAugmentRunsWrapper.tsx

+  if (!props.pipeline.loaded) {
+    return null;
+  }
+  if (props.pipeline.loaded && pipelineData.length < 1) {


Suggested change

if (props.pipeline.loaded && pipelineData.length < 1) {

if (pipelineData.length === 0) {

Best be explicit and not mathematically correct 🙂

Also, don't need to include the inverse of previous boolean statement.

andrewballantyne · 2020-12-16T19:05:59Z

...ines-plugin/src/components/pipelines/list-page/__tests__/PipelineAugmentRunsWrapper.spec.tsx

+  it('Should not render if the firehose call is not yet loaded', () => {
+    expect(wrapper.html()).toBeNull();
+  });


We should always favour loading spinners... There is almost never a good reason to do null if the content is still being processed by a hook or firehose.

andrewballantyne · 2020-12-16T19:07:32Z

.../packages/pipelines-plugin/src/components/pipelines/list-page/PipelineAugmentRunsWrapper.tsx

+  if (!props.pipeline.loaded) {
+    return null;
+  }
+  if (props.pipeline.loaded && pipelineData.length < 1) {
    return (
      <div className="cos-status-box">
        <div className="text-center">No {PipelineModel.labelPlural} Found</div>


This needs to be translated... mind adding this to your PR?

andrewballantyne · 2020-12-16T19:08:15Z

...ines-plugin/src/components/pipelines/list-page/__tests__/PipelineAugmentRunsWrapper.spec.tsx

+
+  it('Should render No Pipelines found text if the data is empty', () => {
+    wrapper.setProps({ pipeline: { data: [], loaded: true } });
+    expect(wrapper.find('div.text-center').text()).toBe('No Pipelines Found');


🤔 I am not happy with these kind of tests... they are brittle and tie themselves to the exact implementation. If we swapped the divs out for PF components, this test would fail, but effectively not useful to us in anyway. If we changed the text, that would also not be ideal.

Perhaps the test here should be you don't render the ListPage 🤔

andrewballantyne · 2020-12-16T19:09:18Z

...ines-plugin/src/components/pipelines/list-page/__tests__/PipelineAugmentRunsWrapper.spec.tsx

+    const firehoseComponent = wrapper.find(Firehose);
+
+    expect(firehoseComponent.exists()).toBeTruthy();


Let's not write tests against how we load data... this test serves just to bind our implementation to Firehose. Really what we want to do is not worry about how it gets the data and that it just renders the needed wrapper.

.../packages/pipelines-plugin/src/components/pipelines/list-page/PipelineAugmentRunsWrapper.tsx

frontend/public/locales/en/public.json

.../packages/pipelines-plugin/src/components/pipelines/list-page/PipelineAugmentRunsWrapper.tsx

...ines-plugin/src/components/pipelines/list-page/__tests__/PipelineAugmentRunsWrapper.spec.tsx

debsmita1 · 2020-12-17T16:12:11Z

/lgtm

andrewballantyne · 2020-12-18T20:39:31Z

...ines-plugin/src/components/pipelines/list-page/__tests__/PipelineAugmentRunsWrapper.spec.tsx

+  });
+
+  it('Should render loader if data not yet loaded', () => {
+    expect(wrapper.find(LoadingBox).exists()).toBeTruthy();


Nit, we should always equate booleans to their boolean value.

Suggested change

expect(wrapper.find(LoadingBox).exists()).toBeTruthy();

expect(wrapper.find(LoadingBox).exists()).toBe(true);

It's not wrong to stay truthy (true is truthy lol), it's just a bit inaccurate on a principle basis.

I'm indifferent if this changes before merging. We need a linter to check for .exists() and .tobeTruthy() on the same expect. Update our Linter ticket: https://issues.redhat.com/browse/ODC-5300

openshift-bot · 2020-12-20T09:39:28Z

/retest

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

openshift-bot · 2020-12-20T13:43:49Z

/retest

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

openshift-bot · 2020-12-20T16:02:36Z

/retest

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

openshift-bot · 2020-12-20T16:43:12Z

/retest

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

openshift-bot · 2020-12-20T18:20:50Z

/retest

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

openshift-bot · 2020-12-20T18:41:04Z

/retest

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

openshift-bot · 2020-12-20T22:27:53Z

/retest

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

openshift-bot · 2020-12-21T00:43:06Z

/retest

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

openshift-ci-robot · 2020-12-21T04:08:45Z

@karthikjeeyar: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test analyze
/test backend
/test e2e-gcp-console
/test frontend
/test images
/test kubevirt-plugin

Use /test all to run the following jobs:

pull-ci-openshift-console-master-analyze
pull-ci-openshift-console-master-backend
pull-ci-openshift-console-master-e2e-gcp-console
pull-ci-openshift-console-master-frontend
pull-ci-openshift-console-master-images

In response to this:

/test ci/prow/e2e-gcp-console

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.

karthikjeeyar · 2020-12-21T04:09:28Z

/test e2e-gcp-console

openshift-bot · 2020-12-21T04:32:51Z

/retest

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

openshift-bot · 2020-12-21T06:13:29Z

/retest

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

openshift-bot · 2020-12-21T08:53:45Z

/retest

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

openshift-bot · 2020-12-21T11:29:43Z

/retest

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

openshift-bot · 2020-12-21T14:31:43Z

/retest

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

openshift-bot · 2020-12-21T19:56:43Z

/retest

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

openshift-bot · 2020-12-21T20:22:44Z

/retest

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

openshift-bot · 2020-12-22T01:11:54Z

/retest

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

openshift-bot · 2020-12-22T01:24:23Z

/retest

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

openshift-bot · 2020-12-22T02:03:25Z

/retest

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

openshift-bot · 2020-12-22T02:16:25Z

/retest

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

openshift-bot · 2020-12-22T02:55:24Z

/retest

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

openshift-bot · 2020-12-22T03:08:23Z

/retest

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

openshift-bot · 2020-12-22T05:31:24Z

/retest

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

openshift-bot · 2020-12-22T06:10:24Z

/retest

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

openshift-bot · 2020-12-22T08:07:25Z

/retest

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

openshift-bot · 2020-12-22T09:25:25Z

/retest

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

openshift-ci-robot · 2020-12-22T09:32:48Z

@karthikjeeyar: All pull requests linked via external trackers have merged:

openshift/console#7547

Bugzilla bug 1907888 has been moved to the MODIFIED state.

In response to this:

Bug 1907888: Fix pipeline list page loader

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 requested review from jerolimov and sahil143 December 15, 2020 07:38

karthikjeeyar changed the title ~~Fix pipeline list page loader~~ Bug 1907888: Fix pipeline list page loader Dec 15, 2020

openshift-ci-robot assigned andrewballantyne Dec 15, 2020

karthikjeeyar force-pushed the pipeline-list-loader branch from 7acbbc7 to a333500 Compare December 16, 2020 09:21

andrewballantyne suggested changes Dec 16, 2020

View reviewed changes

karthikjeeyar force-pushed the pipeline-list-loader branch 2 times, most recently from d387720 to d1660b2 Compare December 17, 2020 11:06

openshift-ci-robot added the component/core Related to console core functionality label Dec 17, 2020

karthikjeeyar force-pushed the pipeline-list-loader branch from d1660b2 to 040bc7f Compare December 17, 2020 12:22

andrewballantyne reviewed Dec 17, 2020

View reviewed changes

frontend/public/locales/en/public.json Outdated Show resolved Hide resolved

karthikjeeyar force-pushed the pipeline-list-loader branch 2 times, most recently from 57b1893 to 9711a9f Compare December 17, 2020 14:02

debsmita1 reviewed Dec 17, 2020

View reviewed changes

.../packages/pipelines-plugin/src/components/pipelines/list-page/PipelineAugmentRunsWrapper.tsx Outdated Show resolved Hide resolved

karthikjeeyar force-pushed the pipeline-list-loader branch from 9711a9f to e1a50cd Compare December 17, 2020 14:26

debsmita1 reviewed Dec 17, 2020

View reviewed changes

...ines-plugin/src/components/pipelines/list-page/__tests__/PipelineAugmentRunsWrapper.spec.tsx Outdated Show resolved Hide resolved

Fix pipeline list page loader

b434822

karthikjeeyar force-pushed the pipeline-list-loader branch from e1a50cd to b434822 Compare December 17, 2020 16:09

openshift-ci-robot assigned debsmita1 Dec 17, 2020

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

andrewballantyne approved these changes Dec 18, 2020

View reviewed changes

openshift-merge-robot merged commit 092f305 into openshift:master Dec 22, 2020

spadgett added this to the v4.7 milestone Jan 7, 2021

	if (props.pipeline.loaded && pipelineData.length < 1) {
	if (pipelineData.length === 0) {

		const firehoseComponent = wrapper.find(Firehose);

		expect(firehoseComponent.exists()).toBeTruthy();

	expect(wrapper.find(LoadingBox).exists()).toBeTruthy();
	expect(wrapper.find(LoadingBox).exists()).toBe(true);

Bug 1907888: Fix pipeline list page loader #7547

Bug 1907888: Fix pipeline list page loader #7547

Conversation

karthikjeeyar commented Dec 15, 2020 • edited

karthikjeeyar commented Dec 15, 2020

openshift-ci-robot commented Dec 15, 2020

andrewballantyne commented Dec 15, 2020

openshift-ci-robot commented Dec 16, 2020

karthikjeeyar commented Dec 16, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Dec 16, 2020

Choose a reason for hiding this comment

andrewballantyne Dec 16, 2020

Choose a reason for hiding this comment

andrewballantyne Dec 16, 2020

Choose a reason for hiding this comment

andrewballantyne Dec 16, 2020

Choose a reason for hiding this comment

andrewballantyne Dec 16, 2020

Choose a reason for hiding this comment

debsmita1 commented Dec 17, 2020

andrewballantyne Dec 18, 2020

Choose a reason for hiding this comment

andrewballantyne Dec 18, 2020 • edited

Choose a reason for hiding this comment

openshift-bot commented Dec 20, 2020

openshift-bot commented Dec 20, 2020

openshift-bot commented Dec 20, 2020

openshift-bot commented Dec 20, 2020

openshift-bot commented Dec 20, 2020

openshift-bot commented Dec 20, 2020

openshift-bot commented Dec 20, 2020

openshift-bot commented Dec 21, 2020

openshift-ci-robot commented Dec 21, 2020

karthikjeeyar commented Dec 21, 2020

openshift-bot commented Dec 21, 2020

openshift-bot commented Dec 21, 2020

openshift-bot commented Dec 21, 2020

openshift-bot commented Dec 21, 2020

openshift-bot commented Dec 21, 2020

openshift-bot commented Dec 21, 2020

openshift-bot commented Dec 21, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-bot commented Dec 22, 2020

openshift-ci-robot commented Dec 22, 2020

karthikjeeyar commented Dec 15, 2020 •

edited

andrewballantyne Dec 18, 2020 •

edited