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

Fix model serving tests in 2.6 #1125

Merged
merged 7 commits into from
Jan 18, 2024

Conversation

bdattoma
Copy link
Contributor

@bdattoma bdattoma commented Jan 16, 2024

Some issues with model serving UI testing faced after the new changes in 2.6.

Likely, we just need to fix the teardown in ods_ci/tests/Tests/400__ods_dashboard/420__model_serving/422__model_serving_llm_UI.robot

Other tentative fixes for timing issues:

  • MM test often fails because the UI doesn't load the model route in time, it's been there from many releases but in 2.6 we've seen a slightly different behavior (a warning msg instead of obscured input element).
  • metrics check in KServe test tends to take some time to fully load, hence adding a Wait until keyword succeedes (both UI and CLI test)

Copy link
Contributor

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
399 0 0 399 100

@bdattoma bdattoma marked this pull request as ready for review January 17, 2024 09:06
@bdattoma bdattoma added needs testing Needs to be tested in Jenkins enhancements Bugfixes, enhancements, refactoring, ... in tests or libraries (PR will be listed in release-notes) labels Jan 17, 2024
${url}= Get Element Attribute xpath://div[.="${model_name} "]${MS_TABLE_ENDPOINT_INPUT} value
SeleniumLibrary.Page Should Contain Element xpath://div[.="${model_name} "]
${route_xpath}= Set Variable xpath://div[.="${model_name} "]${MS_TABLE_ENDPOINT_INPUT}
${loaded}= Run Keyword And Return Status SeleniumLibrary.Wait Until Page Contains Element ${route_xpath} timeout=15s

Check warning

Code scanning / Robocop

Line is too long ({{ line_length }}/{{ allowed_length }}) Warning test

Line is too long (132/120)
@bdattoma bdattoma changed the title Fix model serving tests after changes in 2.6 Fix model serving tests in 2.6 Jan 17, 2024
Comment on lines 199 to 203
${loaded}= Run Keyword And Return Status SeleniumLibrary.Wait Until Page Contains Element ${route_xpath} timeout=15s
IF ${loaded} == ${FALSE}
SeleniumLibrary.Reload Page
SeleniumLibrary.Wait Until Page Contains Element ${route_xpath} timeout=15s
END
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this masking a UI bug? Either the thing should appear without reloading, and then don't reload in the test, or it appears only after a reload, and then always reload. Not both.

Copy link
Member

Choose a reason for hiding this comment

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

I never saw that particular page live, so I'm not sure which, but page reload should IMO never be necessary for a user to do, unless we are workarounding something just in a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's been reported to UI team already this morning, they will look into it. I'm going to add a warning for our tests when this IF branch is used

Copy link
Member

Choose a reason for hiding this comment

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

I'm just afraid that this will get lost among all the warnings we already have in our tests 🙁

Copy link
Contributor Author

@bdattoma bdattoma Jan 17, 2024

Choose a reason for hiding this comment

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

it's a realistic scenario. The alternative was to leave test failing (not worth IMO for this small UI bug). But I'm open to alternative approaches

Copy link
Member

Choose a reason for hiding this comment

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

Addition of the Jira to the code is good for me ATM 🙂 I don't have better proposal now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bdattoma I would suggest to check if model is loaded in inference service then look for the xpath in the dashboard page

Copy link
Contributor Author

@bdattoma bdattoma Jan 18, 2024

Choose a reason for hiding this comment

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

I don't think it would solve, it seems a UI delay in refreshing the data

@bdattoma
Copy link
Contributor Author

bdattoma commented Jan 17, 2024

PR validation

  • Model Serving Smoke (UI): rhods-smoke/4364/ 1 failure due to test environment issue, re-running it. Apart from the failed test, the rest has passed successfully (workaround for MM intermittent issue worked)
    - new run: rhods-smoke/4365/ PASS

@bdattoma bdattoma added verified This PR has been tested with Jenkins and removed needs testing Needs to be tested in Jenkins labels Jan 17, 2024
diegolovison
diegolovison previously approved these changes Jan 17, 2024
jstourac
jstourac previously approved these changes Jan 17, 2024
Copy link
Member

@jstourac jstourac left a comment

Choose a reason for hiding this comment

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

Approving, but have to say that having a warning for that reload page - in case it's an issue indeed - isn't a good approach as I really doubt we will ever notice this warning.

@bdattoma bdattoma dismissed stale reviews from jstourac and diegolovison via eefb9a2 January 17, 2024 15:08
SeleniumLibrary.Page Should Contain Element xpath://div[.="${model_name} "]
${route_xpath}= Set Variable xpath://div[.="${model_name} "]${MS_TABLE_ENDPOINT_INPUT}
${loaded}= Run Keyword And Return Status SeleniumLibrary.Wait Until Page Contains Element ${route_xpath} timeout=15s
IF ${loaded} == ${FALSE}

Check notice

Code scanning / Robocop

'{{ block_name }}' condition can be simplified Note test

'IF' condition can be simplified
@bdattoma
Copy link
Contributor Author

Approving, but have to say that having a warning for that reload page - in case it's an issue indeed - isn't a good approach as I really doubt we will ever notice this warning.

This is a kind of issue which imo is not worth to make the test failing bcos of it.

I'm open to alternatives :)

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@bdattoma bdattoma merged commit 44fb2be into red-hat-data-services:master Jan 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancements Bugfixes, enhancements, refactoring, ... in tests or libraries (PR will be listed in release-notes) verified This PR has been tested with Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants