Fix core-testing.sh #227
Fix core-testing.sh #227
Conversation
WalkthroughThe script Changes
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
scripts/core-testing.sh (1)
262-269: 🛠️ Refactor suggestionUnquoted file paths and backup artifacts
Variables${src_manifest}and${dst_manifest}are unquoted in thejqandteecalls, which can break on paths containing spaces. Additionally,sed -i".sedbck"leaves.sedbckbackup files behind. Please:- jq 'del(.version)' ${src_manifest} | tee ${dst_manifest} + jq 'del(.version)' "${src_manifest}" | tee "${dst_manifest}"And avoid backup files:
- sed -i".sedbck" 's/...//g' … + sed -i 's/...//g' …
♻️ Duplicate comments (5)
scripts/core-testing.sh (5)
81-81: Repeated redundantset +u
Thisset +umirrors the one at line 63. Apply the same cleanup: either remove both or introduceset -u/set +uaround only the blocks that need it.
169-174: Repeated block – venv activation andset +u
This block is essentially the same pattern noted above: disablingnounsetand sourcing/activating the venv. Please consolidate these into shared logic.
204-210: Repeated virtualenv activation
Again the pattern of disablingnounsetand sourcing the venv is repeated here. Consolidate this into a function to make the script more maintainable.
224-227: Repeated virtualenv activation
Yet another instance of sourcing and activating the venv. Extract into a single helper to eliminate duplication.
251-254: Repeated virtualenv activation
The copy-back section also redoes the venv activation—this is the fifth occurrence. Consolidate into a single function.
🧹 Nitpick comments (11)
scripts/core-testing.sh (11)
63-63: Redundantset +u
The script never enablesnounset(set -u), soset +uhas no effect. Consider removing it to avoid confusion or enableset -uat the top if you intend to guard against unbound variables.
115-117: Duplicate reset logic after initial clone
Lines 115–117 repeat the early "reset & checkout" steps (lines 92–94). Consider factoring this into a helper function to DRY up the code and ease future maintenance.
123-128: Overly aggressive Git cleanup
Thestash,stash drop, andgit clean -fdsteps will irreversibly delete untracked files and dropped stashes. If that’s intended, add a comment warning users. Otherwise, consider limiting cleanup (e.g.,git clean -fdXor selectively removing only certain files).
141-147: Repeated virtualenv activation
The script sourcespython-venv.shand then activates the venv again here. This pattern appears multiple times. Extract this into aensure_venvfunction to avoid duplication and reduce the chance of divergence.
148-151: Bootstrapping pip components via shell pipe
Usinggrep | sed | shworks but can be brittle ifscript/bootstrapchanges format. Consider a more robust approach (e.g., parsingscript/bootstrapor using a small Python snippet) to avoid accidental code execution.
178-184: Hidden errors in translation fetch
Redirecting both stdout and stderr to/dev/nullinscript.translationssilences any failures. For better observability, consider redirecting only stdout and letting errors surface:- python3 -m script.translations develop --all > /dev/null 2>&1 + python3 -m script.translations develop --all > /dev/null
188-193: Duplicateuv pip installof the editable package
Theuv pip install -e . ...with constraints appears in both bootstrap (line 150) and here (line 193). You likely only need to install the editable package once. DRY this out to speed up runs and reduce confusion.
218-218: Quote variables inpytestinvocation
Unquoted expansions (${subject},${basedir}) can break if arguments contain spaces or glob characters. Either quote them or build an array of args:pytest "${debug_params}" "${subject}" "tests/components/plugwise_usb/${basedir}" …
234-239: Clean up disabledmypyblock
You’ve commented out themypyinvocation and replaced it with a note. Ifmypyis truly deprecated here, remove the dead code or add aTODOwith a plan to re-enable or remove this in the near future.
271-276: Remove legacy commented-out flags
The commented-out--requirementsflag and its historical notes add noise. If it's no longer needed, delete these lines or consolidate the rationale in a single comment.
288-288: Consider safer pre-commit invocation
Runninggit add -A; pre-commitmutates the working directory. For pre-commit checks, you can use:pre-commit run --all-filesThis avoids unintended staging of files and aligns with common pre-commit usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/core-testing.sh(8 hunks)
🔇 Additional comments (3)
scripts/core-testing.sh (3)
56-61: Sourcing external setup scripts
The environment setup scripts (setup.shandpython-venv.sh) are sourced correctly and SC1091 is properly disabled. No concerns here.
78-78: Definition ofmanualdir
Introducingmanualdirfor caching a manual clone is clear and works as intended. No issues found.
293-293: No actionable change on the closingfi.
| echo "" | ||
| pip install --upgrade -q --disable-pip-version-check "${module}" | ||
| uv pip install --upgrade "${module}" | ||
| fi # pip_prep |
There was a problem hiding this comment.
Improper iteration of manifest requirements
module=$(jq '.requirements[]' ...) may output multiple lines, but uv pip install "${module}" treats the entire block as a single argument. This will fail when there’s more than one requirement. Consider:
jq -r '.requirements[]' ../custom_components/plugwise_usb/manifest.json \
| xargs -L1 uv pip install --upgrade
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
scripts/core-testing.sh (1)
196-200: 🛠️ Refactor suggestion
⚠️ Potential issueFix iteration over manifest requirements
The current code assigns all entries to a singlemodulevariable and runs one install command. If there are multiple requirements, this will fail. Instead, install each requirement individually:-module=$(jq '.requirements[]' ../custom_components/plugwise_usb/manifest.json | tr -d '"') -uv pip install --upgrade "${module}" +jq -r '.requirements[]' ../custom_components/plugwise_usb/manifest.json \ + | xargs -L1 uv pip install --upgrade.pre-commit-config.yaml (1)
90-96: Mirror path resolution improvements intest-pip-prep
Apply the same simplifications from thetest-core-prephook totest-pip-prepto ensure consistency in how the script is invoked.
🧹 Nitpick comments (10)
scripts/core-testing.sh (9)
43-46: Update or clarify the comment referencing plugwise-beta
The comment “Cloned/adjusted code from plugwise-beta” and the note about not using thevenvmay be outdated now that you source and activate it later. Consider clarifying or removing stale commentary to avoid confusion.
56-61: Modularize virtual environment setup
Sourcing external scripts (setup.shandpython-venv.sh) is a solid step toward modularity. Ensure these helper scripts:
- Exit with non-zero on failures.
- Document their expected state changes (e.g., creation vs. activation of the venv).
63-74: Clarify use ofset +uand argument parsing
Disabling-uglobally suppresses unset-variable errors. Instead, consider:
- Explicitly checking for undefined variables.
- Using
[[ $# -eq 2 ]]or[[ -n $VAR ]]for robust argument checks.- Quoting variables in assignments to avoid word-splitting.
77-79: Consider leveraginggit worktreeinstead of manual copy
Maintaining a separatemanual_clone_hadirectory duplicates repository data and complicates syncing. Usinggit worktreecould streamline branch checkouts without full copies and reduce disk usage.
81-102: DRY up initial clone and reuse logic
The logic for cloning and rebasing HA-core repeats the reset/checkout steps in both the first-time clone and reuse paths. Extract common sequences (e.g.,git reset --hard,git checkout,git pull) into a function to reduce duplication and simplify future maintenance.
141-147: Remove redundant venv sourcing
You sourcepython-venv.shand then immediatelysource ./venv/bin/activate. Ifpython-venv.shalready activates the venv, the second step may be redundant. Consolidate to a single activation step to avoid confusion.
148-150: Switch touvfor bootstrap pip installs
Great improvement usinguv pip installfor the HA-core bootstrap. To catch failures within the pipeline, consider enablingpipefail(e.g.,set -o pipefail) so errors ingrep|sed|shpropagate correctly.
224-244: Re-enable or documentmypystatus
mypyis currently disabled with a note. If re-enabling isn’t possible immediately, consider adding a TODO or comment explaining why it’s skipped and when it should be brought back to maintain type safety.
248-261: Streamline non-CI copy-back and manifest edits
The copy-back steps and JSON/Sed operations for non-GitHub Actions could be consolidated or moved to a helper function. Additionally, the-i".sedbck"backup files may clutter the tree—consider cleaning them up after editing..pre-commit-config.yaml (1)
83-90: Refinetest-core-prephook entry
The currententryhardcodesscripts/core-testing.shviabash -c. For readability and maintainability, you can simplify to:entry: bash scripts/core-testing.sh core_prepThis relies on the hook’s
language: scriptcontext and removes the extra-c 'exec env …'wrapper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.pre-commit-config.yaml(3 hunks)scripts/core-testing.sh(8 hunks)
🔇 Additional comments (4)
.pre-commit-config.yaml (4)
5-5: Skip HA core testing hooks in CI as intended
Excludingtest-core-prep,test-pip-prep,testing, andqualityensures pre-commit.ci doesn’t run these long-running steps, while still preserving them for local workflows.
64-67: Validatebiome-lintdependency specification
Switching fromprettiertobiome-lintis a solid improvement. Confirm that@biomejs/biome@1.8.3is the correct npm package and version for this hook to function as expected.
98-104: Confirm exit behavior fortestinghook
Thetestinghook runsscripts/core-testing.sh testingunderGITHUB_ACTIONS=1. Verify that pytest failures properly propagate exit codes so the hook fails on test errors as intended.
104-110: Approvequalityhook configuration
Thequalityhook is correctly set topass_filenames: falseand invokes the updated script. It aligns with the other HA-core testing hooks and fits the local workflow.
| set +u | ||
| cd "${coredir}" || exit | ||
| echo "" | ||
| echo "Ensure HA-core venv" | ||
| # shellcheck disable=SC1091 | ||
| source "./venv/bin/activate" | ||
| echo "" | ||
| echo "Test commencing ..." | ||
| echo "" | ||
| debug_params="" | ||
| if [ ! "${DEBUG}" == "" ] ; then | ||
| debug_params="-rpP --log-cli-level=DEBUG" | ||
| fi | ||
| # shellcheck disable=SC2086 | ||
| pytest ${debug_params} ${subject} --cov=homeassistant/components/plugwise_usb/ --cov-report term-missing -- "tests/components/plugwise_usb/${basedir}" || exit | ||
| pytest ${debug_params} ${subject} tests/components/plugwise_usb/${basedir} --snapshot-update --cov=homeassistant/components/plugwise_usb/ --cov-report term-missing || exit | ||
| fi # testing |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test invocation robustness
- Replace
[ ! "$DEBUG" == "" ]with[[ -n "$DEBUG" ]]for clarity. - Quote expansions in the
pytestcommand to prevent word-splitting:
- if [ ! "${DEBUG}" == "" ] ; then
- debug_params="-rpP --log-cli-level=DEBUG"
- fi
- pytest ${debug_params} ${subject} tests/... --snapshot-update ...
+ if [[ -n "${DEBUG}" ]]; then
+ debug_params=(-rpP --log-cli-level=DEBUG)
+ fi
+ pytest "${debug_params[@]}" "${subject}" tests/... --snapshot-update ...Committable suggestion skipped: line range outside the PR's diff.
| # By default assumes running against 'master' branch of Core-HA | ||
| # as requested by @bouwew for on-par development with the releases | ||
| # version of HA | ||
| # | ||
| # Override by setting BRANCH environment variable to run against | ||
| # a different branch of core | ||
| # | ||
| # Github flows are run against both 'dev' and 'master' | ||
| core_branch="master" | ||
| if [ "x${BRANCH}" != "x" ]; then | ||
| core_branch="${BRANCH}" | ||
| fi | ||
| echo "Working on HA-core branch ${core_branch}" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify default branch and refine override variable
Using master as the default HA-core branch works today, but many repositories (including Home Assistant) are migrating to main. To avoid breakage if master ever disappears, consider:
- Verifying
masterexists and falling back tomain. - Renaming the generic
BRANCHvariable to something more specific likeHA_CORE_BRANCHto prevent collisions.
🏁 Script executed:
#!/usr/bin/env bash
# Verify whether 'master' or 'main' exists on the remote
if git ls-remote --heads https://github.com/home-assistant/core.git master | grep -q master; then
echo "master exists"
elif git ls-remote --heads https://github.com/home-assistant/core.git main | grep -q main; then
echo "main exists"
else
echo "Neither 'master' nor 'main' found on remote"
exit 1
fiLength of output: 127
Ensure robust branch selection and avoid variable collisions
To future‑proof against a migration from master to main and prevent accidental overrides from other scripts, I recommend:
- Detect which default branch actually exists (master → main)
- Rename the generic
BRANCHenv var toHA_CORE_BRANCH
Suggested patch in scripts/core-testing.sh (lines 4–16):
- # By default assumes running against 'master' branch of Core-HA
- core_branch="master"
- if [ "x${BRANCH}" != "x" ]; then
- core_branch="${BRANCH}"
- fi
+ # Determine default HA core branch: prefer 'master', fallback to 'main'
+ if git ls-remote --heads https://github.com/home-assistant/core.git master \
+ | grep -q master; then
+ default_branch="master"
+ elif git ls-remote --heads https://github.com/home-assistant/core.git main \
+ | grep -q main; then
+ default_branch="main"
+ else
+ echo "Error: neither 'master' nor 'main' found on remote"
+ exit 1
+ fi
+
+ # Allow override via HA_CORE_BRANCH (instead of generic BRANCH)
+ core_branch="${HA_CORE_BRANCH:-$default_branch}"
- echo "Working on HA-core branch ${core_branch}"
+ echo "Working on HA-core branch ${core_branch}"• Update CI/docs to use HA_CORE_BRANCH instead of BRANCH.
• Verify downstream jobs/configs set the new variable name accordingly.
Summary by CodeRabbit