fix(gazelle): correct runfiles path handling in gazelle_python_manifest test #3398
Conversation
…st test The gazelle_python_manifest test was failing on Linux but passing on macOS due to incorrect file path handling. The test used $(rootpath) instead of $(rlocationpath) for data files and didn't use the resolved runfiles paths returned by runfiles.Rlocation(). Changes: - Update defs.bzl to use $(rlocationpath) for _TEST_MANIFEST and _TEST_REQUIREMENTS environment variables (matching the existing pattern for _TEST_MANIFEST_GENERATOR_HASH) - Fix test.go to use the resolved paths from runfiles.Rlocation() instead of the unresolved paths at lines 53, 73, and 84-86 This ensures the test works consistently across all platforms by properly using the Bazel runfiles mechanism. Fixes bazel-contrib#3397 Note: Some code generation was assisted by Claude AI. The human author has reviewed, tested, and takes full responsibility for all changes.
Summary of ChangesHello @timfallmk, 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 resolves a persistent failure of the Highlights
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
|
![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 correctly fixes a runfiles path handling issue in the gazelle_python_manifest test by switching from $(rootpath) to $(rlocationpath) in the Bazel rule and using the resolved runfile paths in the Go test. The changes look good and address the problem described. I've added a suggestion to refactor some duplicated logic in the Go test to improve maintainability.
Address code review feedback from Gemini Code Assist bot by creating a getResolvedRunfile helper function. This reduces code duplication and improves error messages by consolidating the logic for: - Getting environment variable values - Validating they are set - Resolving paths through runfiles.Rlocation() Applied the helper to all three environment variables: - _TEST_REQUIREMENTS - _TEST_MANIFEST - _TEST_MANIFEST_GENERATOR_HASH This reduces ~20 lines of duplicated code to 3 function calls and provides more specific error messages that include the env var name and unresolved path. Addresses: bazel-contrib#3398 (comment)
2 participants
Summary
This PR fixes the
gazelle_python_manifest.testfailure on Linux CI by correcting the runfiles path handling in both the Bazel rule definition and the Go test code.Fixes #3397
Problem
The test was failing on Linux but passing on macOS due to inconsistent file path handling:
$(rootpath)instead of$(rlocationpath)in the Bazel ruleSee issue #3397 for full technical details.
Changes
gazelle/manifest/defs.bzl$(rootpath)to$(rlocationpath)for_TEST_MANIFEST$(rootpath)to$(rlocationpath)for_TEST_REQUIREMENTSThis makes them consistent with the existing
_TEST_MANIFEST_GENERATOR_HASHwhich already used$(rlocationpath).gazelle/manifest/test/test.gomanifestPathResolvedinstead ofmanifestPathinmanifestFile.Decode()requirementsPathResolvedinstead ofrequirementsPathinos.Open()manifestPathResolvedinstead ofmanifestPathin error handlingThe test was already calling
runfiles.Rlocation()to resolve the paths, but then wasn't using the resolved values.Testing
Tested on Linux by running:
Result: ✅ PASSED
Notes
_TEST_MANIFEST_GENERATOR_HASHDisclaimer: This is my first contribution to this project, so I'm not entirely certain this is the correct contribution method. Please let me know if any changes to the PR process are needed. Happy to make adjustments!