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

Move Serving and Pipelines suites out of Dashboard folder #1553

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

bdattoma
Copy link
Contributor

first PR part of the test suites refactoring

@bdattoma bdattoma added needs testing Needs to be tested in Jenkins misc Miscelaneus (PR will be listed in release-notes) labels Jun 21, 2024
@bdattoma bdattoma self-assigned this Jun 21, 2024
Copy link
Contributor

github-actions bot commented Jun 21, 2024

Robot Results

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

@manosnoam
Copy link
Contributor

Besides running Smoke to see nothing is broken, lgtm.

@bdattoma
Copy link
Contributor Author

Besides running Smoke to see nothing is broken, lgtm.

yes, didn't get time to do it yet. Will do asap. Ty

FedeAlonso
FedeAlonso previously approved these changes Jun 25, 2024
@bdattoma
Copy link
Contributor Author

bdattoma commented Jun 25, 2024

PR validation:

  • Smoke regression job rhods-ci-pr-test/3005/ PASS
  • Sanity regression job (restricted to the impacted suites) rhods-ci-pr-test/3011 28/107 fails, but don't relate to the changes in this PR
  • dryrun: PASS. If there were issues with Imports, dryrun would fail

@bdattoma bdattoma added verified This PR has been tested with Jenkins ⚠️ high priority review I need this to be reviewed ASAP and removed needs testing Needs to be tested in Jenkins labels Jun 25, 2024
@bdattoma bdattoma requested a review from FedeAlonso June 25, 2024 13:17
@lugi0
Copy link
Contributor

lugi0 commented Jun 25, 2024

If there were issues with Imports, dryrun would fail

are you sure?

Comment on lines 1 to 2
*** Settings ***
Documentation Collection of UI tests to validate the model serving stack for Large Language Models (LLM)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test suite is all about UI tests, wouldn't it make more sense to have it in the 400__ods_dashboard folder?
I remember the beginning of a discussion about this topic but I don't know if we came to a decision in the end

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel the same about 434__data-science-pipelines-ui.robot. Now it is a Dashboard UI test and it's maintained by the Dashboard team. In case it was run by the "Gated Auto-Merger tool" it should be linked to Dashboard source synchronization, not to data-science-pipelines-operator

Related to this, maybe 434__data-science-pipelines-ui.robot should have tag Dashboard-DataSciencePipelines (or DataSciencePipelines-UI) instead of DataSciencePipelines. What do you think @FedeAlonso ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I explained the change of the decision in the thread and in the document of the proposal. It's to avoid having tests sparse: test suite for model mesh is mainly UI based with some mix of CLI. If we move that suire to Dashboard UI we would be losing the model mesh coverage under Serving directory - test coverage of serving would be sparse over 2 folders.

So, to do things consistently over all the components - at least trying - I decided to revert the decision and gather in the same repository the CLI and UI tests of the same component.

jiridanek
jiridanek previously approved these changes Jun 25, 2024
@bdattoma bdattoma requested review from lugi0 and jiridanek June 25, 2024 14:31
@bdattoma bdattoma merged commit 3d280c9 into red-hat-data-services:master Jun 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
misc Miscelaneus (PR will be listed in release-notes) verified This PR has been tested with Jenkins ⚠️ high priority review I need this to be reviewed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants