-
Notifications
You must be signed in to change notification settings - Fork 151
feat: added unit test files to frontend #1312
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
Conversation
ederign
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsun19 thank you so much for the PR. On the PR description, can you please avoid using directly Red Hat issues on the description?
| export const isRedHatRegistryUri = (uri: string): boolean => uri.startsWith('oci://example.io/'); // TODO: Change this to a proper check | ||
|
|
||
| export const isRedHatRegistryUri = (uri: string): boolean => | ||
| uri.startsWith('oci://registry.redhat.io/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid hard code red hat here?
|
/assign @lucferbux |
|
Everything looks great, @rsun19 once you fix that would be ready to go (remember not porting rhoai related code) |
@ederign @lucferbux Should I just remove the whole function then? |
|
Great question tbh, we can parametrize it, change it to "isCompanyUri" and the string for company can be an env variable, so we got a default upstream and then we can override it downstream with |
|
I did something like |
lucferbux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Signed-off-by: rsun19 <[email protected]>
Signed-off-by: rsun19 <[email protected]>
Signed-off-by: rsun19 <[email protected]>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lucferbux The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* added unit test files Signed-off-by: rsun19 <[email protected]> * fixed lint errors Signed-off-by: rsun19 <[email protected]> * updated uri function Signed-off-by: rsun19 <[email protected]> --------- Signed-off-by: rsun19 <[email protected]> Signed-off-by: Taj010 <[email protected]>
* added unit test files Signed-off-by: rsun19 <[email protected]> * fixed lint errors Signed-off-by: rsun19 <[email protected]> * updated uri function Signed-off-by: rsun19 <[email protected]> --------- Signed-off-by: rsun19 <[email protected]> Signed-off-by: Taj010 <[email protected]>
Description
Ported https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/modelRegistrySettings/__tests__/ModelRegistryTableRowStatus.spec.tsx and https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts from the midstream
How Has This Been Tested?
Unit tests passing
Merge criteria:
DCOcheck)ok-to-testhas been added to the PR.If you have UI changes