Skip to content

test(aqua): cover manifest symlink bins precedence#10159

Closed
risu729 wants to merge 2 commits into
jdx:mainfrom
risu729:fix/aqua-symlink-bins-manifest-only
Closed

test(aqua): cover manifest symlink bins precedence#10159
risu729 wants to merge 2 commits into
jdx:mainfrom
risu729:fix/aqua-symlink-bins-manifest-only

Conversation

@risu729

@risu729 risu729 commented May 31, 2026

Copy link
Copy Markdown
Contributor

Stacked on #10187. Target branch remains the default branch.

Summary

  • add regression coverage that Aqua install-manifest symlink_bins survives when current config overrides ordinary Aqua options
  • keep this PR focused on the manifest/config precedence coverage; the Aqua vars canonicalization fix lives in fix(aqua): canonicalize and reject duplicate vars #10187

Details

The install manifest can carry layout-affecting Aqua options from the installed tool. This test covers the case where current config overrides a normal option such as channel, while preserving manifest-only symlink_bins = true so the installed runtime layout remains exposed correctly.

Verification

  • cargo test test_resolve_tool_opts_preserves_aqua_symlink_bins_from_install_manifest
  • git diff --check
  • cargo fmt --all -- --check

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Aqua backend option handling to support and prioritize plain options over vars. prefixed options, and ensures proper serialization of these options in lockfile_options. It also adds corresponding unit tests to verify the precedence of plain variables and the preservation of options like symlink_bins from the install manifest. There are no review comments, so I have no feedback to provide.

@greptile-apps

greptile-apps Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds regression test coverage for the Aqua install-manifest/config option precedence logic, specifically ensuring that symlink_bins from an install manifest survives when user config overrides a regular option like channel.

  • src/backend/aqua.rs: Adds two new unit tests covering AquaVarOptionPriority — one for reading prefixed (vars.channel) keys and one asserting that a plain channel key beats a vars.channel prefixed key. The existing nested-precedence test is also refactored to reorder BTreeMap insertions, making it explicit that insertion order is irrelevant.
  • src/config/mod.rs: Adds test_resolve_tool_opts_preserves_aqua_symlink_bins_from_install_manifest, an async test that constructs a minimal Config with a channel=stable config option and a manifest carrying channel=manifest plus symlink_bins=true, then asserts channel resolves from Config while symlink_bins is retained from the InstallManifest.

Confidence Score: 5/5

This PR is safe to merge — all changes are test-only additions with no production logic touched.

Both changed files add or refactor unit tests exclusively. The new tests correctly model the intended precedence rules (nested > plain > prefixed, config > manifest) and the assertions match the documented behavior. No production code paths are modified.

No files require special attention.

Important Files Changed

Filename Overview
src/backend/aqua.rs Adds two new unit tests for AquaVarOptionPriority precedence (prefixed-var reads and plain-beats-prefixed), and reorders insertions in the existing nested-precedence test to demonstrate BTreeMap order-independence — test-only changes with no production logic.
src/config/mod.rs Adds test_resolve_tool_opts_preserves_aqua_symlink_bins_from_install_manifest, an async integration-style unit test that verifies symlink_bins from the install manifest is preserved while a conflicting channel key is correctly overridden by config — test-only change.

Reviews (5): Last reviewed commit: "test(aqua): cover manifest symlink bins ..." | Re-trigger Greptile

Comment thread src/backend/aqua.rs Outdated
@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 01551654-8040-417a-9dda-a2b54704f75a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@risu729 risu729 changed the title fix(aqua): resolve prefixed vars with manifest opts fix(aqua): preserve manifest symlink bins and vars May 31, 2026
@risu729

risu729 commented May 31, 2026

Copy link
Copy Markdown
Contributor Author

CI follow-up after 9231d92:

  • All non-task-completion checks are green or skipped as expected, including lint, build-ubuntu, build-windows, windows-unit, windows-e2e, benchmark, registry, release, and Socket.
  • The remaining failures are e2e-2 and e2e-3. Both fail in task completion tests, not Aqua code:
    • tasks/test_task_completion: usage complete-word returned mise.toml mise.toml and mise.usage.kdl mise.usage.kdl where the test expected bare candidates.
    • tasks/test_task_completion_global_cd: usage complete-word returned alpha alpha, beta beta, gamma gamma where the test expected bare candidates.
  • The PR diff only touches Aqua option/lockfile handling and a config regression test. I also checked recent main test runs; the latest main test run is still pending and the immediately preceding ones were cancelled, so there is no completed base failure to cite yet. Based on the logs, these failures appear unrelated to this PR.

This comment was generated by an AI coding assistant.

@risu729 risu729 force-pushed the fix/aqua-symlink-bins-manifest-only branch from 9231d92 to 45c8624 Compare May 31, 2026 22:01
@risu729 risu729 changed the title fix(aqua): preserve manifest symlink bins and vars test(aqua): cover manifest symlink bins precedence May 31, 2026
@risu729 risu729 force-pushed the fix/aqua-symlink-bins-manifest-only branch from 45c8624 to ed8ec34 Compare May 31, 2026 22:04
@risu729

risu729 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Closing this PR because its remaining branch delta is test-only (src/config/mod.rs). The original behavior change, including canonical Aqua vars in lock/cache identity and rejecting ambiguous duplicate var spellings, is now handled by #10187.

This comment was generated by an AI coding assistant.

@risu729 risu729 closed this Jun 2, 2026
@risu729 risu729 deleted the fix/aqua-symlink-bins-manifest-only branch June 13, 2026 15:29
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