Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes test setup into a shared test_common.sh, removes duplicated boilerplate from individual test scripts, and makes test paths agnostic by introducing temporary directories and using TFENV_CONFIG_DIR in cleanup and directory checks.
- Added
test_common.shto consolidate TFENV_ROOT determination, helpers sourcing, and PATH setup - Updated individual tests to source
test_common.shand replaced hard-coded paths withTFENV_CONFIG_DIRor temporary directories - Enhanced
cleanup()inlib/helpers.shto verifyTFENV_CONFIG_DIRbefore removing test artifacts
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_use_minrequired.sh | Removed inline TFENV_ROOT boilerplate, now sources test_common.sh |
| test/test_use_latestallowed.sh | Same boilerplate removal and common setup sourcing |
| test/test_uninstall.sh | Sourced common setup and switched version-dir checks to use TFENV_CONFIG_DIR |
| test/test_symlink.sh | Sourced common setup; replaced fixed /tmp path with mktemp -d and added trap for cleanup |
| test/test_list.sh | Removed inline setup and sources test_common.sh |
| test/test_install_and_use.sh | Boilerplate removed, now sources common setup |
| test/test_common.sh | New shared setup file for TFENV_ROOT, helpers, and PATH |
| test/run.sh | Refactored to source test_common.sh |
| lib/helpers.sh | Updated cleanup() to check for TFENV_CONFIG_DIR and delete from that directory |
Comments suppressed due to low confidence (2)
test/test_uninstall.sh:56
TFENV_CONFIG_DIRis referenced here but never initialized intest_common.sh, causing this check to always fail. Consider defining and exportingTFENV_CONFIG_DIRintest_common.shto point to an isolated config directory for tests.
[ -d "${TFENV_CONFIG_DIR}/versions" ] || exit 1
test/test_common.sh:3
- Tests and the cleanup routine rely on
TFENV_CONFIG_DIRbut it isn't set here. Add an initialization, e.g.:export TFENV_CONFIG_DIR="$(mktemp -d)"; trap 'rm -rf "${TFENV_CONFIG_DIR}"' EXIT.
# Common test setup header
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.