Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NETOBSERV-967 skip ticks when popups are open #375

Merged
merged 5 commits into from
Sep 6, 2023

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Aug 25, 2023

Auto refresh seems to retrigger the popups rendering and resets their content.

This PR simply skip tick when any popup is open to:

  • avoid useless rendering
  • keep perfs stable while in popups

@memodi
Copy link
Contributor

memodi commented Aug 25, 2023

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 25, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:3b66276

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=3b66276 make set-plugin-image

@memodi
Copy link
Contributor

memodi commented Aug 25, 2023

this solves one issue where column is automatically unselected during auto-refresh, however, this is still giving problems when selecting group columns in cypress tests, I have this code: https://github.com/openshift/openshift-tests-private/blob/master/frontend/tests/netobserv/netflow_table.cy.ts#L134-L181 , I unskipped this test and uncommented lines after comment for 967 bug the test still gets page rendered where group selected columns does not exist.

here you can see the K8s-owner object column goes missing despite selected:

image

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 28, 2023
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage: 57.14% and project coverage change: -0.32% ⚠️

Comparison is base (1fe04e7) 57.70% compared to head (75de06a) 57.38%.
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #375      +/-   ##
==========================================
- Coverage   57.70%   57.38%   -0.32%     
==========================================
  Files         166      167       +1     
  Lines        7710     7803      +93     
  Branches      932      941       +9     
==========================================
+ Hits         4449     4478      +29     
- Misses       2993     3056      +63     
- Partials      268      269       +1     
Flag Coverage Δ
uitests 58.31% <57.14%> (+0.28%) ⬆️
unittests 54.82% <ø> (-1.96%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
web/src/components/netflow-traffic.tsx 55.83% <0.00%> (+0.05%) ⬆️
web/src/components/modals/columns-modal.tsx 72.72% <100.00%> (+0.41%) ⬆️
...eb/src/components/modals/overview-panels-modal.tsx 73.01% <100.00%> (+0.43%) ⬆️

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jpinsonneau
Copy link
Contributor Author

this solves one issue where column is automatically unselected during auto-refresh, however, this is still giving problems when selecting group columns in cypress tests, I have this code: https://github.com/openshift/openshift-tests-private/blob/master/frontend/tests/netobserv/netflow_table.cy.ts#L134-L181 , I unskipped this test and uncommented lines after comment for 967 bug the test still gets page rendered where group selected columns does not exist.

here you can see the K8s-owner object column goes missing despite selected:

image

Yes I remember we had that discussion but I'm still not able to reproduce using our cypress 🤔
I have added a similar method than your approach that uses checkboxes and it still works on my side
https://github.com/netobserv/network-observability-console-plugin/pull/375/files#diff-bbb920770f884a6314eb2ee333b15fd3cc85ad22e10ce7431b6d090e5fc9be76R112-R116

We are using cypress 12.5.1 whereas you are on 11.2.0. Could it be related ?

Also tried a page reload but still works 2b0ae80

@jpinsonneau
Copy link
Contributor Author

Also added an extra check in the popup useEffect 1a878fa

Can you please let me know if it solve your issue @memodi ?

@memodi
Copy link
Contributor

memodi commented Aug 28, 2023

/ok-to-test

Can you please let me know if it solve your issue @memodi ?

Sure, I can try. I see you're checking for 4 columns, could you check in your add for #K8S_OwnerObject, then I reload() and check for [data-test=th-K8S_OwnerObject] > .pf-c-table__button existence where I face most challenges.

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 28, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:94dc4c1

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=94dc4c1 make set-plugin-image

@memodi
Copy link
Contributor

memodi commented Aug 28, 2023

Also added an extra check in the popup useEffect 1a878fa

@jpinsonneau I am seeing much better results with this commit, I ran test ~10 times where I also removed reload() and wait() in between and it passed all the times. Thank you!

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Aug 28, 2023
export const url = 'http://localhost:9000/netflow-traffic';
// export const url = 'http://localhost:9001';
// export const url = 'http://localhost:9000/netflow-traffic';
export const url = 'http://localhost:9001';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that an intentional change or something you forgot to switch back to original?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly use standalone for these so I guess it would be great to keep it as default

jotak
jotak previously approved these changes Aug 29, 2023
Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a small question

@openshift-ci
Copy link

openshift-ci bot commented Sep 5, 2023

New changes are detected. LGTM label has been removed.

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 5, 2023
@jpinsonneau
Copy link
Contributor Author

I had to add a case to fix testing since useEffect are not correctly managed by enzyme
75de06a

In the end it allows columns to be updated at first start independently of the popup state (open / closed).

@openshift-ci
Copy link

openshift-ci bot commented Sep 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 84e175b into netobserv:main Sep 6, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm qe-approved QE has approved this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants