pipelines icon indicating copy to clipboard operation
pipelines copied to clipboard

test: Fix failing SDK test by replacing mock.patch with subprocess for actual build

Open DharmitD opened this issue 1 year ago • 5 comments

Description of your changes: Resolves: https://github.com/kubeflow/pipelines/issues/11038

  • Removed the mock.patch on subprocess.run.
  • Added a direct call to subprocess.run to build the KFP package.
  • Introduced error handling with a helpful error message if the package build fails.
  • Kept the rest of the test logic intact.

The original mock.patch.object wasn’t working because it prevented the real build from happening, which was necessary for the test to pass. Replacing it with a direct call to subprocess.run ensures the KFP package is actually built, resolving the issue. This approach allows the test to reflect the real build environment and provides clearer error reporting if something goes wrong during the build process.

SDK test GHA passed on this PR: https://github.com/kubeflow/pipelines/actions/runs/11284347350/job/31385382544?pr=11285 Alternatively, you could test locally by running the following command from the repo's root directory:

pytest sdk/python/kfp --ignore=sdk/python/kfp/deprecated --cov=kfp

Checklist:

DharmitD avatar Oct 10 '24 23:10 DharmitD

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign connor-mccarthy for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

google-oss-prow[bot] avatar Oct 11 '24 00:10 google-oss-prow[bot]

ready for review. cc: @chensun @gregsheremeta @connor-mccarthy @hbelmiro

DharmitD avatar Oct 14 '24 16:10 DharmitD

what was the mock.patch.object supposed to be doing, why wasn't it working, and how does calling a build via subprocess act as a valid replacement? (The commit message and PR should explain this, too.)

gregsheremeta avatar Oct 14 '24 19:10 gregsheremeta

New changes are detected. LGTM label has been removed.

google-oss-prow[bot] avatar Oct 14 '24 21:10 google-oss-prow[bot]

what was the mock.patch.object supposed to be doing, why wasn't it working, and how does calling a build via subprocess act as a valid replacement? (The commit message and PR should explain this, too.)

Done, added some details on this in the commit message and the PR description.

DharmitD avatar Oct 14 '24 21:10 DharmitD

/rerun-all

VaniHaripriya avatar Nov 04 '24 22:11 VaniHaripriya