Skip to content

test: Stabilize CLI runner subprocess path#6560

Merged
franciscojavierarceo merged 1 commit into
masterfrom
codex/fix-cli-runner-pythonpath
Jun 26, 2026
Merged

test: Stabilize CLI runner subprocess path#6560
franciscojavierarceo merged 1 commit into
masterfrom
codex/fix-cli-runner-pythonpath

Conversation

@franciscojavierarceo

Copy link
Copy Markdown
Member

What changed

  • Normalizes the environment used by CliRunner subprocesses in unit tests.
  • Adds the parent pytest process sys.path entries to PYTHONPATH before invoking feast.cli.cli in a child Python process.
  • Applies the same environment to both subprocess.run(...) and subprocess.Popen(...) helper paths.

Why

The unit-test workflow flaked on unit-test-python (3.12, ubuntu-latest) while setting up test_push_source_does_not_exist. The parent pytest process had imported Feast successfully, but the child feast apply subprocess failed before the test body with:

ModuleNotFoundError: No module named 'google.protobuf.timestamp_pb2'

That points at the subprocess import environment rather than the test assertion. Passing through the parent process import path makes the CLI subprocess use the same repo/site-packages view as pytest.

Validation

  • uv run ruff check sdk/python/tests/utils/cli_repo_creator.py
  • uv run python -m pytest sdk/python/tests/unit/test_feature_server.py::test_push_source_does_not_exist -q
  • subprocess smoke: from google.protobuf.timestamp_pb2 import Timestamp using CliRunner._subprocess_env()

Note: local setup required make install-python-dependencies-ci PYTHON_VERSION=3.12 because this machine does not have a python shim for the Makefile's default version detection.

Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
@franciscojavierarceo franciscojavierarceo marked this pull request as ready for review June 26, 2026 15:27
@franciscojavierarceo franciscojavierarceo requested a review from a team as a code owner June 26, 2026 15:27
@franciscojavierarceo franciscojavierarceo merged commit 141c08c into master Jun 26, 2026
27 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79d89d66e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

parent_paths = [path for path in sys.path if path]
existing_pythonpath = env.get("PYTHONPATH")
if existing_pythonpath:
parent_paths.append(existing_pythonpath)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve child PYTHONPATH precedence

In workflows where PYTHONPATH intentionally contains an empty/current-directory entry for the CLI cwd (for example, .github/workflows/unit_tests.yml exports sdk/python:${PYTHONPATH}:$SITE_PACKAGES), appending the raw existing value after the parent sys.path moves parent site-packages and repo-root paths ahead of the temporary feature repo. Since cli_check_repo() only appends the feature repo path later, imports for repo-local providers or feature definition modules can resolve to an installed/root module with the same name, so CliRunner no longer exercises the isolated repo. Keep the original PYTHONPATH entries first, or add only parent-only paths after the child cwd entry.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant