test: Fix failing SDK test by replacing mock.patch with subprocess for actual build #11285
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
DharmitD
force-pushed
the
sdk-test
branch
5 times, most recently
from
5881e68
to
1fa1b4a
Compare
|
ready for review. |
|
what was the |
…ctual build The original test used mock.patch to mock subprocess.run, which prevented the real KFP package build from occurring. This caused the test to fail as it couldn't find the expected artifacts. Replaced the mock with a direct subprocess.run call to ensure the package is built correctly during the test. This resolves the issue by performing the actual build process, making the test more reliable and surfacing any build errors clearly. Signed-off-by: ddalvi <[email protected]>
|
New changes are detected. LGTM label has been removed. |
DharmitD
changed the title
Done, added some details on this in the commit message and the PR description. |
| def test_dockerfile_can_contain_custom_kfp_package(self): | ||
| component = _make_component( | ||
| func_name='train', target_image='custom-image') | ||
| _write_components('components.py', component) | ||
| package_dir = os.path.dirname(os.path.dirname(self.current_dir)) | ||
|
|
||
| # suppresses large stdout from subprocess that builds kfp package | ||
| with mock.patch.object( |
There was a problem hiding this comment.
I think it is actually the intention to mock subprocess runs, as unit tests should generally be isolated and dependency-free. Subprocesses introduce external dependencies that can make tests flaky and harder to debug.
Why was the mock not working?
There was a problem hiding this comment.
The original error message indicates that the test can't find the built KFP wheel file in the temp directory.
This is because the mock of subprocess.run doesn’t perform the actual build process (e.g., running python setup.py bdist_wheel), it doesn't create the wheel file.
While the mock ensures subprocess.run doesn't actually execute, it fails to account for the necessary file outputs (i.e., .whl file).
In this case mock wasn't building out the wheel file entirely. Replacing by subprocess.run did it.
|
/rerun-all |
Description of your changes:
Resolves: #11038
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:
Checklist: