Skip to content

✅ Add Fish shell completion tests for colon options#1475

Merged
svlandeg merged 4 commits into
fastapi:masterfrom
Mohamed-Elwasila:add-fish-completion-tests
May 19, 2026
Merged

✅ Add Fish shell completion tests for colon options#1475
svlandeg merged 4 commits into
fastapi:masterfrom
Mohamed-Elwasila:add-fish-completion-tests

Conversation

@Mohamed-Elwasila

Copy link
Copy Markdown
Contributor

Summary

This PR adds the missing Fish shell completion tests that were marked as TODO in test_completion_option_colon.py.

Changes

  • Added test_completion_colon_fish_all() - Tests all completions are shown
  • Added test_completion_colon_fish_partial() - Tests partial matching (e.g., "alpine" matches both variants)
  • Added test_completion_colon_fish_single() - Tests single match completion

Testing

All 3 new tests pass, and all 16 tests in the file pass without issues.

Fixes the TODO comment at line 219 in tests/test_completion/test_completion_option_colon.py.

@svlandeg svlandeg added feature New feature, enhancement or request repo / tests Involving the CI / test suite labels Jan 6, 2026
@ADiTyaRaj8969

Copy link
Copy Markdown
Contributor

Hi @Mohamed-Elwasila, thanks for picking up this TODO 👍

I checked out this branch locally on Windows (Python 3.12, fresh venv from pyproject.toml) and ran the full file. All 16 tests pass, including the 3 new fish ones:

tests/test_completion/test_completion_option_colon.py ... 16 passed in 26.64s

A few observations from the review:

Things that look right

  • The three tests cleanly mirror the existing *_zsh_* ones, which is exactly the right pattern — same fixture (colon_example.py), same _TYPER_COMPLETE_ARGS semantics (trailing-space ⇒ incomplete == "", no trailing-space ⇒ last token is the incomplete).
  • The _TYPER_COMPLETE_FISH_ACTION="get-args" value matches what FishComplete.complete() checks for in typer/_completion_classes.py, so the tests are exercising the same code path that fish actually triggers.
  • Removing the # TODO: tests for complete_fish comment is the right move — the TODO is now resolved by the new tests.

One suggestion that would strengthen the tests (optional — not blocking)

The current assert "alpine:hello" in result.stdout style verifies that the value appears, but it doesn't verify the fish-specific output format. FishComplete.format_completion produces value\thelp when help is present, and bare value when it isn't. So for the _all case you could additionally assert that the help text appears for the items in colon_example.image_desc that have descriptions:

assert "latest alpine image" in result.stdout          # help for alpine:latest
assert "fake image: for testing" in result.stdout       # help for alpine:hello

That would catch a regression in format_completion that the current value-only assertions would miss. Same idea applies to the zsh tests in this file, fwiw — they have the same gap — so it might make sense as a separate small follow-up rather than expanding this PR's scope.

Sorry the PR has been sitting for a while — bumping it for visibility.

@svlandeg svlandeg changed the title Add Fish shell completion tests for colon options ✅ Add Fish shell completion tests for colon options May 19, 2026

@svlandeg svlandeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @Mohamed-Elwasila, thanks so much for addressing this TODO! I don't remember why I had it, I feel like I might have just forgot it back when we were working on #988.

Anyway, good to see this resolved now, and the structure you've used is consistent with the other tests in the same module. I've also updated the PR with the suggestions from @ADiTyaRaj8969 that were merged earlier in a separate PR for the other tests (#1767).

@ADiTyaRaj8969 : thanks for the review! It's useful. It does read like it's been (partly) LLM-generated. That's fine, but please in the future disclose any AI usage up front, both for reviewing and for creating PRs 🙏

Thanks again both!

@svlandeg
svlandeg merged commit d20905f into fastapi:master May 19, 2026
22 checks passed
@svlandeg svlandeg added internal Internal changes and removed feature New feature, enhancement or request labels May 19, 2026
@svlandeg svlandeg removed their assignment May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Internal changes repo / tests Involving the CI / test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants