[FEATURE] Extract skip_download logic into testable helpers

[sergio-sisternes-epam]

Is your feature request related to a problem? Please describe.

Unit tests for install skip-download logic (test_install_update_refs.py, added in #550) validate reimplemented helper functions that mirror install.py conditions, rather than exercising the production code paths directly. This means the test suite can pass even if the real condition in install.py changes but the mirrored helper is not updated.

Identified during Copilot review of #550.

Describe the solution you'd like

Either:

• (a) Extract the skip_download and locked_ref decision logic into shared helper functions used by both install.py and the tests, or
• (b) Write integration-level unit tests that call _install_apm_dependencies() with GitHubPackageDownloader mocked (similar to tests/unit/test_install_command.py) to assert update behavior end-to-end without network access.

Option (a) is simpler but requires extracting logic from a closure. Option (b) provides stronger guarantees but requires more mock setup.

Describe alternatives you've considered

• Keep the current mirrored-logic approach (adequate for a 1-line boolean expression, and the tests document the expected truth table clearly).
• Add a comment in install.py referencing the test file so maintainers know to update both.

Additional context

The current tests are functional and catch regressions in the condition semantics. This is a testing quality improvement, not a bug.

Related: #550

[github-actions]

Triage decision

accept

Proposed labels

area/testing
type/refactor
status/accepted
priority/low

Milestone

0.9.4

Suggested next action

Extract the skip_download / locked_ref decision logic into shared helper functions usable by both install.py and its tests, or add integration-level tests that mock GitHubPackageDownloader end-to-end.

Suggested issue comment

Thanks for filing this, `@sergio-sisternes-epam`!

The concern is valid: mirrored-logic tests validate a copy of the condition, not the production condition itself. If the condition in `install.py` changes and the mirror is not updated, the tests pass silently while behavior regresses.

Both options are sound:
- Option (a): extract the condition into a shared helper -- simpler, avoids closure awkwardness if the logic is a small boolean expression.
- Option (b): integration-level tests with `GitHubPackageDownloader` mocked -- stronger guarantee but more mock setup.

Option (a) is preferred if the extracted function is <=10 lines with clear inputs; option (b) if the logic touches multiple internal state variables that are hard to surface cleanly.

Next step: pick one option and open a draft PR referencing this issue and #550.

Per-lens notes (collapsed)

DevX UX Expert -- User-Need Reviewer

The user surface here is contributor DX: APM's test suite should exercise production code paths, not re-implementations. The framing is clear -- mirrored-logic tests are a known anti-pattern in unit testing. No end-user-facing behavior changes; this is a test quality improvement for contributors.

Supply Chain Security Expert -- Risk-Surface Reviewer

No risk-surface implication. Extracting the skip_download decision into a shared helper does not change the security boundary; it improves test fidelity for the install pipeline. If anything, stronger test coverage of the download skip logic reduces the risk of a silent behavioral regression.

OSS Growth Hacker -- Contributor-Tone Reviewer

Not activated -- sergio-sisternes-epam is a COLLABORATOR with extensive contribution history; no tone adjustment for a new-contributor welcome is needed.

Python Architect -- Architecture Reviewer

Not activated -- extracting a small boolean condition from a closure into a module-level helper is a routine refactor. No cross-cutting design decision, no interface change, no schema impact. Option (a) is idiomatic Python and feasibility is high.

Doc Writer -- Documentation Reviewer

Not activated -- no user-facing documentation change implied by a test structure improvement.

APM CEO -- Triage Arbiter

Clear accept. Testing quality improvements that make the install pipeline's skip logic verifiable against production code are exactly the kind of hygiene work that keeps APM's CI trustworthy. Priority/low -- not urgent, but worth doing before the download pipeline grows more complexity. Milestone 0.9.4.

{
  "decision": "accept",
  "decision_detail": "",
  "theme": null,
  "areas": ["area/testing"],
  "type": "type/refactor",
  "status": "status/accepted",
  "priority": "priority/low",
  "preserved_labels": [],
  "milestone": "0.9.4",
  "next_action": "Extract the skip_download / locked_ref decision logic into shared helper functions usable by both install.py and its tests, or add integration-level tests that mock GitHubPackageDownloader end-to-end.",
  "comment_markdown": "Thanks for filing this, `@sergio-sisternes-epam`!\n\nThe concern is valid: mirrored-logic tests validate a copy of the condition, not the production condition itself. If the condition in `install.py` changes and the mirror is not updated, the tests pass silently while behavior regresses.\n\nBoth options are sound:\n- Option (a): extract the condition into a shared helper -- simpler, avoids closure awkwardness if the logic is a small boolean expression.\n- Option (b): integration-level tests with `GitHubPackageDownloader` mocked -- stronger guarantee but more mock setup.\n\nOption (a) is preferred if the extracted function is <=10 lines with clear inputs; option (b) if the logic touches multiple internal state variables that are hard to surface cleanly.\n\nNext step: pick one option and open a draft PR referencing this issue and #550."
}

---

Triage status: agentic proposal pending human ratification.
Silence is approval. Maintainers can:

• Override any label or milestone above by editing it directly --
  human edits are authoritative and will not be reverted on
  subsequent runs.
• Re-trigger triage by applying the status/needs-triage label, or
  by removing status/triaged to enroll the issue in the next
  daily sweep.

Posted by the Triage Panel workflow. See .apm/skills/apm-triage-panel for the panel skill.

Generated by Triage Panel · ● 2.2M · ◷