Skip to content

tests: add smoke coverage for shell completion script#3112

Open
Vaishnav88sk wants to merge 7 commits intoNetflix:masterfrom
Vaishnav88sk:tests/smoke-tests-for-shell-completion
Open

tests: add smoke coverage for shell completion script#3112
Vaishnav88sk wants to merge 7 commits intoNetflix:masterfrom
Vaishnav88sk:tests/smoke-tests-for-shell-completion

Conversation

@Vaishnav88sk
Copy link
Copy Markdown

PR Type

  • Bug fix
  • New feature
  • Core Runtime change (higher bar -- see CONTRIBUTING.md)
  • Docs / tooling
  • Refactoring

Summary

Add a bash smoke test for metaflow-complete.sh to ensure shell completion is loadable and returns expected top-level commands. This catches completion script regressions early in CI and provides a direct local test command for contributors.

Issue

Fixes #3107

Fixes #

Reproduction

Runtime: local (Linux bash)

Commands to run:

# run only the new smoke test
python -m pytest test/unit/test_shell_completion.py -v
# optional: run via tox unit env
tox -e unit -- test/unit/test_shell_completion.py

Where evidence shows up:

Before (error / log snippet)
No dedicated automated smoke coverage existed for `metaflow-complete.sh`.
Completion regressions could pass CI undetected until user-facing breakage.
After (evidence that fix works)
test/unit/test_shell_completion.py::test_metaflow_completion_script_smoke PASSED

Sample completion candidates returned:
code
configure
develop
help
status
tutorials

Root Cause

The completion script is user-facing shell glue that was not covered by a focused automated smoke test. Without CI/runtime validation of script sourcing and returned candidates, regressions in completion behavior can slip through normal test paths.

Why This Fix Is Correct

The test validates the critical behavior contract directly:

  • script can be sourced in bash
  • completion is registered for metaflow
  • completion output contains expected top-level commands

This is minimal and scoped: it adds only a targeted smoke test plus a contributor doc command, without changing CLI/runtime behavior.

Failure Modes Considered

  1. Bash loads script but no completion registration (complete -p metaflow check fails)
  2. Completion function runs but expected command candidates are missing (assertion includes actionable missing set and full output)

Tests

  • Unit tests added/updated
  • Reproduction script provided (required for Core Runtime)
  • CI passes
  • If tests are impractical: explain why below and provide manual evidence above

Non-Goals

  • No changes to completion algorithm or CLI command behavior.
  • No cross-shell expansion beyond the current bash smoke scope.
  • No refactor of packaging or test runner infrastructure.

AI Tool Usage

  • No AI tools were used in this contribution
  • AI tools were used (describe below)

Used an AI coding assistant (Claude) to draft the test structure and PR text. All generated content was reviewed, adjusted, and validated manually (including command-level behavior checks and syntax checks) before submission.

Signed-off-by: Vaishnav88sk <vaishnavsk8804@gmail.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 14, 2026

Greptile Summary

Adds a bash smoke test for metaflow-complete.sh that sources the completion script, verifies the registration with complete -p metaflow, and asserts that expected top-level commands appear in COMPREPLY. All three P1 concerns raised in earlier review rounds (fixed-path race condition, unbound wrapper_path in finally, wrong wrapper filename causing click to miss _METAFLOW_COMPLETE) have been addressed in this revision.

Confidence Score: 5/5

Safe to merge — only a P2 style suggestion remains; no correctness or reliability issues on the primary target platform (Linux bash 4+).

All previously-flagged P1 issues are resolved. The one remaining finding (empty-array nounset behaviour on bash < 4.4) only affects failure-mode diagnostics, not the happy path or CI correctness on modern Linux.

No files require special attention.

Important Files Changed

Filename Overview
test/unit/test_shell_completion.py New smoke test for bash completion; all three P1 concerns from prior review rounds are resolved (unique temp dir, no stale finally block, wrapper correctly named metaflow). One P2 remains: ${COMPREPLY[@]} under set -u may produce a confusing nounset error on bash < 4.4 when empty.

Reviews (6): Last reviewed commit: "tests: use stable metaflow wrapper name ..." | Re-trigger Greptile

Comment thread test/unit/test_shell_completion.py
Comment thread test/unit/test_shell_completion.py Outdated
Signed-off-by: Vaishnav88sk <vaishnavsk8804@gmail.com>
Comment thread test/unit/test_shell_completion.py Outdated
Signed-off-by: Vaishnav88sk <vaishnavsk8804@gmail.com>
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.

Add smoke test for shell completion script behavior

1 participant