Skip to content

test: Add Integration tests for Kubernetes Native API Migration script #12180

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2025
Merged

test: Add Integration tests for Kubernetes Native API Migration script #12180

merged 1 commit into from
Oct 7, 2025

Conversation

@VaniHaripriya
Copy link
Contributor

Description of your changes:

  • Created a GitHub workflow and corresponding tests to verify the Kubernetes Native API migration script.

Checklist:

@google-oss-prow
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch 4 times, most recently from 4ba3634 to 79a3a72 Compare August 25, 2025 18:52
@VaniHaripriya VaniHaripriya marked this pull request as ready for review August 25, 2025 19:11
@google-oss-prow google-oss-prow bot requested a review from rimolive August 25, 2025 19:11
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch from 79a3a72 to 3f26607 Compare August 26, 2025 14:10
nsingla
nsingla reviewed Aug 26, 2025
View reviewed changes
Copy link
Contributor

@nsingla nsingla left a comment

Choose a reason for hiding this comment

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

I feel like a lot of the assertions are hardcoded and only verifying the presence of files/pipelines.
We should probably write these tests in pytest rather than Python Unit test, and use of fixture for set up and tear down can make things much easier.
Can you please add some code comments explaining what the tests are suppose to do? May be add a link it to the test in the proposal per test
Also, curious to know why were these tests written in python as oppose to Golang?

Since this is probably just 1 off thing, we can live by with the hard coded assertions but we should still add assertions to compare whole objects rather than just the presence of pipelines/runs/versions

@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch from 3f26607 to f030956 Compare August 27, 2025 20:25
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch from f030956 to 779385d Compare August 27, 2025 21:09
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch 4 times, most recently from 669e0ad to 520e383 Compare September 4, 2025 15:46
DharmitD
DharmitD reviewed Sep 5, 2025
View reviewed changes
nsingla
nsingla reviewed Sep 8, 2025
View reviewed changes
test/kfp-kubernetes-native-migration-tests/create_test_pipelines.py Outdated

KFP_ENDPOINT = os.environ.get('KFP_ENDPOINT', 'http://localhost:8888')

def serialize_object_for_comparison(obj):
Copy link
Contributor

Choose a reason for hiding this comment

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

would json.dump or json.dumps, not suffice ? Any particular reason for this method?

@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch 2 times, most recently from 65ee14b to 0cfcec5 Compare September 16, 2025 15:14
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch 7 times, most recently from 8c20e48 to 7cd4030 Compare October 6, 2025 16:37
nsingla
nsingla approved these changes Oct 6, 2025
View reviewed changes
Copy link
Contributor

@nsingla nsingla left a comment

Choose a reason for hiding this comment

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

lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 6, 2025
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch from 7cd4030 to d38c672 Compare October 6, 2025 18:33
@google-oss-prow google-oss-prow bot removed the lgtm label Oct 6, 2025
nsingla
nsingla approved these changes Oct 6, 2025
View reviewed changes
Copy link
Contributor

@nsingla nsingla left a comment

Choose a reason for hiding this comment

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

lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 6, 2025
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch from d38c672 to 2ef3252 Compare October 7, 2025 16:47
@google-oss-prow google-oss-prow bot removed the lgtm label Oct 7, 2025
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch 4 times, most recently from 1f87862 to 7868389 Compare October 7, 2025 18:49
@VaniHaripriya VaniHaripriya force-pushed the k8s-native-migration-script-tests branch from 7868389 to 2252d9b Compare October 7, 2025 19:34
@nsingla
Copy link
Contributor

nsingla commented Oct 7, 2025

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 7, 2025
@HumairAK
Copy link
Collaborator

HumairAK commented Oct 7, 2025

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK

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

@HumairAK HumairAK added the ci-passed All CI tests on a pull request have passed label Oct 7, 2025
@google-oss-prow google-oss-prow bot merged commit 9e24f70 into kubeflow:master Oct 7, 2025
52 checks passed
JerT33 pushed a commit to JerT33/pipelines_dev that referenced this pull request Oct 8, 2025
@VaniHaripriya @JerT33
test: Add Integration tests for Kubernetes Native API Migration script (
0e92da3 
kubeflow#12180)

Signed-off-by: VaniHaripriya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@droctothorpe droctothorpe Awaiting requested review from droctothorpe

@mprahl mprahl Awaiting requested review from mprahl

@rimolive rimolive Awaiting requested review from rimolive

3 more reviewers

@DharmitD DharmitD DharmitD left review comments

@alyssacgoins alyssacgoins alyssacgoins left review comments

@nsingla nsingla nsingla approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nsingla nsingla

Labels

approved ci-passed All CI tests on a pull request have passed lgtm size/XXL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@VaniHaripriya @nsingla @HumairAK @DharmitD @alyssacgoins