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.

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions sdk/python/kfp/cli/component_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
# limitations under the License.
"""Tests for `components` command group in KFP CLI."""
import contextlib
import functools
import os
import pathlib
import subprocess
Expand Down Expand Up @@ -579,27 +578,28 @@ def test_existing_dockerfile_can_be_overwritten(self):
COPY . .
'''))

@unittest.skip(
"Skipping this test as it's failing. Refer to https://github.com/kubeflow/pipelines/issues/11038"
)
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(
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

subprocess,
'run',
new=functools.partial(subprocess.run, capture_output=True)):
result = self.runner.invoke(
self.cli,
[
'build',
str(self._working_dir), f'--kfp-package-path={package_dir}'
],
)
try:
subprocess.run(['python3', 'setup.py', 'bdist_wheel'],
cwd=package_dir,
check=True,
capture_output=True)
except subprocess.CalledProcessError as e:
print(f'Failed to build KFP package: {e.stderr.decode()}')
raise

result = self.runner.invoke(
self.cli,
[
'build',
str(self._working_dir), f'--kfp-package-path={package_dir}'
],
)
self.assertEqual(result.exit_code, 0)
self._docker_client.api.build.assert_called_once()
self.assert_file_exists('Dockerfile')
Expand Down
Loading