Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Task/APPENG-2738: NIM Cypress E2E tests #3298
Task/APPENG-2738: NIM Cypress E2E tests #3298
Changes from all commits
82dc694
777fbf1
88fe97b
dddb3fc
683bf47
353ae78
f44825c
534222b
da51470
1a0b155
a7826d2
5dc4c2c
5a87a50
963d077
ff3d67b
c34551f
bbd1688
394820e
4d62576
3858254
a5d6858
35f9c15
917d012
3e2b3c2
a955014
0c519b9
19d20f6
26195c1
98eabbe
abdd860
bd9fa01
b01fe1b
06baa5e
9dea313
80af75e
a9007d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Same here -- another gap we already had.
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.
I'll log a bug for this... this is a common component we failed to properly annotate, shouldn't be one you folks have to make use of. Leave as-is.
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.
I wonder if these have a chance of conflicting with the StorageSize +/-
Hopefully in the improvement issues I'll log we can make sure this is covered too.
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.
I'll log a bug for this... this is a common component we failed to properly annotate, shouldn't be one you folks have to make use of. Leave as-is.
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.
Although these are not likely to change, they feel like they might be brittle based on how the selectors are written.
No action needed atm, but I wonder if we can't log something to improve these gaps in the future.
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.
I don't think this will select anything -- I can't find
section-overview
test id in the code outside of this selector.Looking for classes and going through divs is definitely a fast-track to brittle tests. If you're trying to select an element specifically on a page, we need a better data-testid on it.
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.
I does selects, the test works:
I couldn't find a testid on the deployed model card, using the parent based on the calsses was the only way I was able to select the card. Can you suggest something else please?
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.
This feels like it doesn't belong here -- KServe Specific stuff in a projects file? Hmmm... I wonder if this doesn't belong better in the
modelServing.ts
page component file?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.
Maybe we named it incorrectly, but we use it to select deployed model records from the models table in the Models tab. See test called should list the deployed model in Models tab.
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.
@andrewballantyne there's a decent amount of model serving related duplication between the projects page tests and the model serving page tests, since we have some of the same functionality in 3 places (project details overview tab, project details models tab, model serving page). It maybe should be cleaned up at the same time as some of the refactoring @lucferbux and @emilys314 have mentioned doing as part of the KServe Raw Deployment work.