Skip to content

Decouple hotyvalidate from tests.unit.utils - #1145

Merged
dosaboy merged 1 commit into
canonical:mainfrom
wilkmar:decouple_hotyvalidate_f_tests
Oct 8, 2026
Merged

dosaboy merged 1 commit into
canonical:mainfrom
wilkmar:decouple_hotyvalidate_f_tests

Conversation

@wilkmar

@wilkmar wilkmar commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Move the scenario-test loading needed by the standalone validator into hotsos.core.ycheck.engine.common instead of importing the unit-test utilities and constructing TemplatedTestGenerator instances.

Add TEST_TEMPLATE_SCHEMA and load_test_def() to share YAML loading, empty-template rejection and top-level key validation with TemplatedTestGenerator. Add resolve_target_scenario_path() to map a test under /tests/scenarios to its scenario under /scenarios, honouring target-name and optionally reusing an already-loaded test.

Use YDefsLoader discovery and these helpers in hotyvalidate, removing all references to tests.unit.utils. Centralize definitions-root initialization in configure_defs_dir(), shared by both validation methods and the CLI. Preserve an existing HotSOSConfig.plugin_yaml_defs value; otherwise prefer HOTSOS_DEFS_DIR and fall back to <HOTSOS_ROOT>/defs. Raise RuntimeError when no configuration is available, with the CLI reporting the error and exiting nonzero without a traceback.

Add regression tests for configuration precedence, root fallback, missing configuration, and direct invocation of both validation methods with scenario-test discovery and target-name mapping.

Assisted-By: Claude Opus 4.8

Copilot AI lite review requested due to automatic review settings September 28, 2026 13:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Copilot review overview

Review effort: Lite
Findings: 2 High severity

Open (2)

Comment thread hotsos/core/ycheck/engine/common.py
Comment thread hotsos/core/ycheck/engine/common.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (2)

@wilkmar
wilkmar requested a review from dosaboy September 28, 2026 15:37
@wilkmar
wilkmar force-pushed the decouple_hotyvalidate_f_tests branch from a933d1f to 1414795 Compare September 28, 2026 15:38
@wilkmar
wilkmar force-pushed the decouple_hotyvalidate_f_tests branch from 1414795 to 8be8ef7 Compare October 7, 2026 08:39
@dosaboy
dosaboy requested a balanced review from Copilot October 7, 2026 11:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The validator no longer rejects scenario tests whose resolved target does not exist.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

Comment thread tools/validation/hotyvalidate.py
@dosaboy

dosaboy commented Oct 7, 2026

Copy link
Copy Markdown
Member

lgtm but need to fix the issue raised by copilot

@wilkmar
wilkmar force-pushed the decouple_hotyvalidate_f_tests branch from 8be8ef7 to 9143c75 Compare October 7, 2026 12:56
@wilkmar
wilkmar requested a balanced review from Copilot October 7, 2026 13:37
@wilkmar
wilkmar force-pushed the decouple_hotyvalidate_f_tests branch 2 times, most recently from 9143c75 to 4e92803 Compare October 7, 2026 13:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Invalid definitions paths can produce false-success validation, and several unrelated documents should be split from this change.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment thread tools/validation/hotyvalidate.py Outdated
Move the scenario-test loading needed by the standalone validator into
hotsos.core.ycheck.engine.common instead of importing the unit-test
utilities and constructing TemplatedTestGenerator instances.

Add TEST_TEMPLATE_SCHEMA and load_test_def() to share YAML loading and
top-level key validation with TemplatedTestGenerator. Reject empty or
non-mapping YAML values with InvalidFileFormatError before inspecting keys.
Preserve KeyError for unknown keys and YAML parser errors for malformed
input.

Add resolve_target_scenario_path() to map tests under
<defs>/tests/scenarios to scenarios under <defs>/scenarios, honouring
target-name and optionally reusing an already-loaded test. Require an
absolute test path, normalize it and check directory containment with
commonpath before loading the test. Restrict non-null target-name overrides
to non-empty string basenames, rejecting separators, dot components and
NUL characters.

Use YDefsLoader discovery and these helpers in hotyvalidate, removing all
references to tests.unit.utils. Centralize definitions-root initialization
in configure_defs_dir(), shared by both validation methods and the CLI.
Preserve an existing HotSOSConfig.plugin_yaml_defs value; otherwise prefer
HOTSOS_DEFS_DIR and fall back to <HOTSOS_ROOT>/defs. Raise RuntimeError
when no configuration is available, with the CLI reporting the error and
exiting nonzero without a traceback.

Add regression tests for configuration precedence, root fallback, missing
configuration, and direct invocation of both validation methods. Cover
template types and keys, nested target mapping, normalized paths, path
escapes and invalid target-name overrides.

Assisted-By: Claude Opus 4.8
Signed-off-by: Marcin Wilk <marcin.wilk@canonical.com>
@wilkmar
wilkmar force-pushed the decouple_hotyvalidate_f_tests branch from 4e92803 to eeab730 Compare October 8, 2026 08:21
@wilkmar
wilkmar requested a balanced review from Copilot October 8, 2026 08:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The refactoring preserves existing behavior while adding appropriate validation and regression coverage.

1 open finding

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

@dosaboy
dosaboy merged commit 66c00ed into canonical:main Oct 8, 2026
8 checks passed
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.

3 participants