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

Remove cypress suppression of uncaught exceptions #3063

Merged

Conversation

manaswinidas
Copy link
Contributor

@manaswinidas manaswinidas commented Aug 5, 2024

Closes: https://issues.redhat.com/browse/RHOAIENG-10715
Closes: https://issues.redhat.com/browse/RHOAIENG-7565
Closes: https://issues.redhat.com/browse/RHOAIENG-11444
Closes: https://issues.redhat.com/browse/RHOAIENG-11847

Description

These snippets had been introduced to work around a Patternfly bug with ExpandableSection patternfly/patternfly-react#10410 This has been fixed - removing these suppressions because they can hide other uncaught exceptions.

How Has This Been Tested?

  • Run Cypress test files modelVersionArchive.cy.ts and registeredModelArchive.cy.ts and check whether they run fine.

Test Impact

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@mturley
Copy link
Contributor

mturley commented Aug 5, 2024

@manaswinidas it looks like we're still getting the exception - I think we need to upgrade PatternFly to pull in the fix.

According to this PR comment it was released in @patternfly/[email protected] but we're still on 5.3.3.

@manaswinidas
Copy link
Contributor Author

Oops should have checked that. The tests are passing locally for me. Also, why is registeredModelsArchive.cy.ts not failing?

@mturley
Copy link
Contributor

mturley commented Aug 5, 2024

good question... looking into it. also I'm not sure why tests are passing for you locally... do you have the newer PF installed somehow?

@mturley
Copy link
Contributor

mturley commented Aug 5, 2024

I'm confused by the test failing on "after each" hook for "Restore from archive version details", I don't see an afterEach in there anywhere

@mturley
Copy link
Contributor

mturley commented Aug 5, 2024

The tests are passing for me locally too... I think maybe there's a rendering race condition and the exception isn't necessarily always thrown. Maybe we can just upgrade PF and make sure it passes and that's sufficient.

@manaswinidas
Copy link
Contributor Author

@mturley @patternfly/[email protected] is still in pre-release according to npm versions history

@mturley
Copy link
Contributor

mturley commented Aug 13, 2024

/hold
holding this until PatternFly releases 5.4.0 and we can upgrade to that version in this PR. The Jira is blocked until that happens - The PatternFly team says the release is expected today. If there are further delays we'll need to roll over the issue to the next sprint.

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Aug 13, 2024
@mturley
Copy link
Contributor

mturley commented Aug 22, 2024

Once PF releases and we unblock this, let's make sure to also remove the newly introduced instance of this in https://github.com/opendatahub-io/odh-dashboard/pull/3116/files#diff-cfd373dd2cc4bd02bbf6aa15837b2b82aed88e0ef783079334d738c8b22d6ceaR206-R208

Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

Thanks @manaswinidas ! This was a fun one huh

Copy link
Contributor

openshift-ci bot commented Aug 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mturley

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

@mturley
Copy link
Contributor

mturley commented Aug 28, 2024

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Aug 28, 2024
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.22%. Comparing base (4a0626e) to head (2e0fc1d).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...t/tables/pipelineRun/CustomMetricsColumnsModal.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3063      +/-   ##
==========================================
+ Coverage   80.84%   85.22%   +4.38%     
==========================================
  Files        1243     1244       +1     
  Lines       27099    27117      +18     
  Branches     7144     7152       +8     
==========================================
+ Hits        21907    23111    +1204     
+ Misses       5192     4006    -1186     
Files with missing lines Coverage Δ
frontend/src/components/SimpleSelect.tsx 93.47% <ø> (+8.69%) ⬆️
.../screens/RegisterModel/RegisteredModelSelector.tsx 100.00% <ø> (+6.25%) ⬆️
...t/tables/pipelineRun/CustomMetricsColumnsModal.tsx 7.14% <0.00%> (+4.76%) ⬆️

... and 595 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a0626e...2e0fc1d. Read the comment docs.

@openshift-merge-bot openshift-merge-bot bot merged commit 3174a74 into opendatahub-io:main Aug 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants