fix: resolve fish test path correctly with fishtape - #1071
Conversation
The issue was that (status dirname) returns unexpected values when fish test files are run via fishtape in CI. This caused the path resolution to fail, resulting in 'Unknown command' errors. Fix: Use relative path ../../home-manager/programs/fish/functions directly instead of relying on status dirname, since fishtape runs with the test file's directory as the current working directory.
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR updates the Elixir package version from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses a critical path resolution bug affecting Fish shell tests when executed in a Continuous Integration environment using Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRFixed an issue with fish test path resolution in CI when using What changed?Modified the fish test path resolution to use the relative path Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a path resolution issue in the fish shell tests that was causing failures in CI. The fix involves using a static relative path instead of a dynamic one with (status dirname). Additionally, the Elixir version is pinned to ensure build consistency. While the fix is effective, it has been applied across a large number of test files, leading to significant code duplication. My review focuses on this maintainability issue and suggests a refactoring to centralize the path definition in a shared test helper script.
| set fn ../../home-manager/programs/fish/functions | ||
| source $fn/_cliproxyapi_function.fish |
There was a problem hiding this comment.
While this change fixes the path resolution issue, it introduces significant code duplication by repeating the path definition across more than 40 test files. This will make future maintenance difficult, as any change to the directory structure would require updating all these files.
A better approach would be to centralize this path definition in a single test helper file. For example, you could create a spec/fish/test_helper.fish with the following content:
# spec/fish/test_helper.fish
set -g TEST_FUNCTIONS_PATH ../../home-manager/programs/fish/functionsThen, you could replace these two lines in every test file with something like this:
source test_helper.fish # Assuming test_helper.fish is in the same directory
source $TEST_FUNCTIONS_PATH/_cliproxyapi_function.fishThis refactoring would greatly improve the maintainability of the test suite.
There was a problem hiding this comment.
Pull request overview
This PR aims to fix CI failures in the fish function test suite by changing how spec files locate and source fish functions when executed via fishtape.
Changes:
- Update
spec/fish/*_test.fishfiles to source functions using a fixed relative path instead of(status dirname). - Pin the Home Manager Elixir package selection from
elixirtoelixir_1_19.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/fish/fish_user_key_bindings_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_zpo_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_zmo_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_zdo_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_two_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_tsw_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_tss_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_tsk_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_tsh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_tpo_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_tmo_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_tdo_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_ssh_add_github_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_pixeh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_pixe_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_ocxeh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_ocxe_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_kyberm_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_kyberd_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_kyber_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_hm_load_env_file_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_grcr_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_grco_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_gco_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_shell_history_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_preview_name_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_preview_cmd_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_git_worktree_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_git_branch_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_ghq_picker_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_file_picker_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_directory_picker_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fzf_chrome_history_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_fish_shortcuts_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_dev_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_coxelh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_coxel_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_coxeh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_coxe_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_clxeh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_clxe_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_clwxeh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_clwxe_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_cltxeh_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_cltxe_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| spec/fish/_cliproxyapi_function_test.fish | Switch functions directory resolution to a fixed relative path for fishtape runs |
| home-manager/programs/elixir/default.nix | Pin Elixir package from elixir to elixir_1_19 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| @@ -1,4 +1,4 @@ | |||
| set fn (status dirname)/../../home-manager/programs/fish/functions | |||
| set fn ../../home-manager/programs/fish/functions | |||
There was a problem hiding this comment.
This hard-coded relative path assumes the current working directory is spec/fish (so that ../../home-manager/... resolves correctly). However, make fish-test invokes fishtape spec/fish/*_test.fish from the repo root, so this becomes brittle and may break depending on whether fishtape changes directories internally. Consider deriving the functions directory from the repo root (or passing it in via an environment variable / adjusting the test runner to cd spec/fish first) so path resolution is robust across runners.
| elixir_1_19 | ||
| elixir-ls |
There was a problem hiding this comment.
This PR is described as fixing fishtape path resolution, but this change pins Elixir to elixir_1_19, which appears unrelated to the fish test failure. Please either split this into a separate PR/commit or update the PR description to explain why this version change is included here (and how it relates to the reported issue).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/fish/_fzf_shell_history_test.fish (1)
1-1: Consider centralizing the sharedfnbase path in one test helper.This same literal path is duplicated across many specs; extracting it once will reduce future churn if test layout changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/fish/_fzf_shell_history_test.fish` at line 1, The literal base path "../../home-manager/programs/fish/functions" assigned to the variable fn is duplicated; extract it into a single test helper (e.g., a shared spec helper that exports a FN_BASE variable or a get_fn_path function) and update this test to source that helper and use the shared symbol (fn or FN_BASE/get_fn_path) instead of the hardcoded string so all specs reference the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/fish/_fzf_shell_history_test.fish`:
- Line 1: The literal base path "../../home-manager/programs/fish/functions"
assigned to the variable fn is duplicated; extract it into a single test helper
(e.g., a shared spec helper that exports a FN_BASE variable or a get_fn_path
function) and update this test to source that helper and use the shared symbol
(fn or FN_BASE/get_fn_path) instead of the hardcoded string so all specs
reference the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc23fa23-d8c7-476b-a5a6-7da4429587b1
📒 Files selected for processing (47)
home-manager/programs/elixir/default.nixspec/fish/_cliproxyapi_function_test.fishspec/fish/_cltxe_function_test.fishspec/fish/_cltxeh_function_test.fishspec/fish/_clwxe_function_test.fishspec/fish/_clwxeh_function_test.fishspec/fish/_clxe_function_test.fishspec/fish/_clxeh_function_test.fishspec/fish/_coxe_function_test.fishspec/fish/_coxeh_function_test.fishspec/fish/_coxel_function_test.fishspec/fish/_coxelh_function_test.fishspec/fish/_dev_function_test.fishspec/fish/_fish_shortcuts_test.fishspec/fish/_fzf_chrome_history_test.fishspec/fish/_fzf_directory_picker_test.fishspec/fish/_fzf_file_picker_test.fishspec/fish/_fzf_ghq_picker_test.fishspec/fish/_fzf_git_branch_test.fishspec/fish/_fzf_git_worktree_test.fishspec/fish/_fzf_preview_cmd_test.fishspec/fish/_fzf_preview_name_test.fishspec/fish/_fzf_shell_history_test.fishspec/fish/_gco_function_test.fishspec/fish/_grco_function_test.fishspec/fish/_grcr_function_test.fishspec/fish/_hm_load_env_file_test.fishspec/fish/_kyber_function_test.fishspec/fish/_kyberd_function_test.fishspec/fish/_kyberm_function_test.fishspec/fish/_ocxe_function_test.fishspec/fish/_ocxeh_function_test.fishspec/fish/_pixe_function_test.fishspec/fish/_pixeh_function_test.fishspec/fish/_ssh_add_github_test.fishspec/fish/_tdo_function_test.fishspec/fish/_tmo_function_test.fishspec/fish/_tpo_function_test.fishspec/fish/_tsh_function_test.fishspec/fish/_tsk_function_test.fishspec/fish/_tss_function_test.fishspec/fish/_tsw_function_test.fishspec/fish/_two_function_test.fishspec/fish/_zdo_function_test.fishspec/fish/_zmo_function_test.fishspec/fish/_zpo_function_test.fishspec/fish/fish_user_key_bindings_test.fish
|
Closing this PR to replace with corrected fix on new branch |
The issue was that (status dirname) returns unexpected values when fish test files are run via fishtape in CI. This caused the path resolution to fail, resulting in 'Unknown command' errors.
Fix: Use relative path ../../home-manager/programs/fish/functions directly instead of relying on status dirname, since fishtape runs with the test file's directory as the current working directory.
Summary by cubic
Fixes fishtape path resolution in fish tests to stop "Unknown command" errors in CI. Also pins Elixir to
elixir_1_19for consistent builds.Bug Fixes
../../home-manager/programs/fish/functionsinstead of(status dirname).Dependencies
elixirtoelixir_1_19in Home Manager for a stable toolchain.Written for commit eed18e2. Summary will update on new commits.