[Test Improver] test: add coverage tests for primitives discovery and parser by danielmeppiel · Pull Request #254 · microsoft/apm

danielmeppiel · 2026-03-12T01:18:38Z

🤖 Test Improver — automated AI assistant focused on improving tests.

Goal

Increase test coverage for primitives/discovery.py and primitives/parser.py, two core modules involved in finding and parsing APM primitive files (chatmodes, instructions, contexts, skills).

Approach

Added 54 focused unit tests in a new file tests/unit/primitives/test_discovery_parser.py, covering branches that were reachable but had no existing test paths.

Covered paths

Area	What's tested
`parse_skill_file`	Name from frontmatter; name derived from parent directory when missing
`parse_primitive_file`	Unknown extension raises `ValueError`
`_extract_primitive_name`	Structured dir variants (instructions, context, memory); `.apm/agents/` fallback
`_is_context_file`	`.apm/memory/` and `.github/memory/` detected; other dirs not matched
`validate_primitive`	Wrapper delegates correctly
`_discover_local_skill`	SKILL.md found; not found; parse error warns and skips
`_discover_skill_in_directory`	SKILL.md in dep dir; missing; parse error path
`scan_directory_with_source`	No `.apm` dir (SKILL.md fallback); `.apm` dir with instructions; parse error warning
`get_dependency_declaration_order`	No `apm.yml`; alias dep; virtual GitHub subdir/collection; virtual ADO subdir/collection; single-part repo_url fallbacks; transitive deduplication; exception → `[]` + warning
`scan_local_primitives`	`apm_modules/` exclusion; parse error warns and continues
`_is_readable`	Readable file; permission denied; binary/non-UTF8 file
`_should_skip_directory`	All skip patterns (`.git`, `node_modules`, `__pycache__`, etc.); normal dirs not skipped
`_is_under_directory`	Under and not-under cases
`find_primitive_files`	Non-existent base dir; duplicate file deduplication across patterns

Coverage impact

Module	Before	After
`primitives/discovery.py`	78%	97%
`primitives/parser.py`	72%	76%

Measured via pytest tests/unit/ --cov=apm_cli.primitives.discovery --cov=apm_cli.primitives.parser.

Note: 4 lines in discovery.py remain uncovered in the full-suite coverage run (lines 140, 297, 316, 333 — all collection.add_primitive() success-path calls). These are covered in isolation but appear uncovered in the full suite due to a pre-existing YAML global-state pollution issue that causes some earlier tests to corrupt PyYAML's constructor table under pytest-cov instrumentation. This is not introduced by this PR.

Test status

54 new tests: 54 passed
Full unit suite: 1482 passed, 1 failed (pre-existing ANSI test)

Reproducibility

uv sync --extra dev
uv run pytest tests/unit/primitives/test_discovery_parser.py -v
uv run pytest tests/unit/ -q

Generated by Daily Test Improver · ◷

To install this agentic workflow, run
gh aw add githubnext/agentics/workflows/daily-test-improver.md@b87234850bf9664d198f28a02df0f937d0447295

Cover previously untested paths in discovery.py (78%→97%) and parser.py (72%→76%): - parse_skill_file: name-from-frontmatter and name-from-parent-dir paths - Unknown primitive type raises ValueError - _extract_primitive_name: structured dir variants and fallbacks - _is_context_file: memory dir detection - validate_primitive wrapper - _discover_local_skill / _discover_skill_in_directory with SKILL.md - scan_directory_with_source: no-.apm, success, and error paths - get_dependency_declaration_order: no-apm.yml, alias, virtual-GitHub, virtual-ADO, single-part repo_url, and transitive dedup branches - scan_local_primitives: apm_modules exclusion and parse-error path - _is_readable: readable, permission-denied, and binary-file cases - _should_skip_directory: all skip patterns and normal directories - _is_under_directory and find_primitive_files deduplication 54 new tests added. 1482 unit tests pass (1 pre-existing ANSI failure). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…rage-da40935ebde67a0a

Copilot

Pull request overview

This PR adds a new unit test suite to increase coverage for apm_cli.primitives.discovery and apm_cli.primitives.parser, focusing on previously untested branches around primitive parsing, dependency ordering, and filesystem scanning edge cases.

Changes:

Added tests/unit/primitives/test_discovery_parser.py with extensive branch-coverage tests for discovery + parsing helpers.
Added targeted tests for dependency declaration ordering (including virtual package path formats) and discovery error-handling paths.

Copilot · 2026-03-12T21:21:06Z

tests/unit/primitives/test_discovery_parser.py

+        """Files in .apm/agents/ fall through to fallback (agents/ not in structured subdirs list).
+        The fallback strips .md giving 'myagent.agent'."""
+        path = Path("/project/.apm/agents/myagent.agent.md")
+        name = _extract_primitive_name(path)
+        # 'agents' is not in the structured subdirs list, so fallback strips .md only
+        self.assertEqual(name, "myagent.agent")
+
+    def test_agent_md_in_github_agents_dir(self):
+        """Files in .github/agents/ also fall through to fallback."""
+        path = Path("/project/.github/agents/reviewer.agent.md")
+        name = _extract_primitive_name(path)
+        self.assertEqual(name, "reviewer.agent")


The agent-name assertions here expect _extract_primitive_name() to return values like reviewer.agent for *.agent.md files. That makes the name inconsistent with other primitives (e.g., .chatmode.md strips the type suffix) and appears to contradict the CLI docs example apm compile --chatmode architect (implying the name should be architect, not architect.agent). Consider updating _extract_primitive_name() to strip .agent.md (and treat .apm/agents / .github/agents as structured dirs), then update these tests accordingly so they enforce the documented behavior.

Suggested change

"""Files in .apm/agents/ fall through to fallback (agents/ not in structured subdirs list).

The fallback strips .md giving 'myagent.agent'."""

path = Path("/project/.apm/agents/myagent.agent.md")

name = _extract_primitive_name(path)

# 'agents' is not in the structured subdirs list, so fallback strips .md only

self.assertEqual(name, "myagent.agent")

def test_agent_md_in_github_agents_dir(self):

"""Files in .github/agents/ also fall through to fallback."""

path = Path("/project/.github/agents/reviewer.agent.md")

name = _extract_primitive_name(path)

self.assertEqual(name, "reviewer.agent")

"""Files in .apm/agents/ are treated as structured agent primitives.

The '.agent.md' suffix is stripped, yielding just the agent name."""

path = Path("/project/.apm/agents/myagent.agent.md")

name = _extract_primitive_name(path)

self.assertEqual(name, "myagent")

def test_agent_md_in_github_agents_dir(self):

""".github/agents/ is also treated as a structured agent primitive directory."""

path = Path("/project/.github/agents/reviewer.agent.md")

name = _extract_primitive_name(path)

self.assertEqual(name, "reviewer")

Copilot · 2026-03-12T21:21:07Z

tests/unit/primitives/test_discovery_parser.py

+        os.chmod(path, 0o000)
+        try:
+            result = _is_readable(path)
+            self.assertFalse(result)
+        finally:
+            os.chmod(path, 0o644)


test_unreadable_file_returns_false uses chmod 0o000 to simulate unreadability. This is unreliable across platforms (e.g., Windows permission semantics) and can also fail when tests run with elevated privileges where the file remains readable. A more stable approach is to mock open() (or Path.open) to raise PermissionError for this path, so the test is deterministic in CI.

Suggested change

os.chmod(path, 0o000)

try:

result = _is_readable(path)

self.assertFalse(result)

finally:

os.chmod(path, 0o644)

# Simulate unreadable file by forcing PermissionError when opening.

with patch.object(Path, "open", side_effect=PermissionError):

result = _is_readable(path)

self.assertFalse(result)

Copilot · 2026-03-12T21:21:07Z

tests/unit/primitives/test_discovery_parser.py

+import os
+import stat
+import tempfile
+import unittest
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+from apm_cli.primitives.discovery import (
+    _discover_local_skill,
+    _discover_skill_in_directory,
+    _is_readable,
+    _is_under_directory,
+    _should_skip_directory,
+    discover_primitives,
+    find_primitive_files,
+    get_dependency_declaration_order,
+    scan_directory_with_source,
+    scan_local_primitives,
+)


This import block includes unused imports (stat) and unused imported symbols (discover_primitives). If/when the repo runs linting (or contributors run static analysis), these will be flagged. Please remove unused imports or add usage.

Copilot · 2026-03-12T21:21:07Z

tests/unit/primitives/test_discovery_parser.py

+    def test_unreadable_skill_md_warns_and_skips(self):
+        """parse error on SKILL.md is caught, printed as warning, skipped."""
+        skill_path = Path(self.tmp) / "SKILL.md"
+        _write(skill_path, SKILL_CONTENT)
+        collection = PrimitiveCollection()
+        with patch(
+            "apm_cli.primitives.discovery.parse_skill_file",
+            side_effect=ValueError("bad"),
+        ):
+            import io
+            from contextlib import redirect_stdout
+
+            buf = io.StringIO()
+            with redirect_stdout(buf):
+                _discover_local_skill(self.tmp, collection)
+            self.assertIn("Warning", buf.getvalue())
+        self.assertEqual(len(collection.skills), 0)


The test name/docstring says "unreadable" SKILL.md, but the test is actually exercising the "parse_skill_file raises" path (a parse error), not file readability. Renaming the test (and docstring) to reflect the behavior under test will avoid confusion when debugging failures.

danielmeppiel · 2026-03-12T21:32:38Z

@copilot address pr feedback properly

Copilot · 2026-03-12T21:32:45Z

@danielmeppiel I've opened a new pull request, #268, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>

test: address PR review feedback on primitives discovery/parser tests

danielmeppiel added automation testing labels Mar 12, 2026

danielmeppiel added 2 commits March 12, 2026 22:07

Merge branch 'main' into test-assist/primitives-discovery-parser-cove…

1c18708

…rage-da40935ebde67a0a

Merge branch 'main' into test-assist/primitives-discovery-parser-cove…

d019fdd

…rage-da40935ebde67a0a

danielmeppiel marked this pull request as ready for review March 12, 2026 21:16

Copilot AI review requested due to automatic review settings March 12, 2026 21:16

Copilot started reviewing on behalf of danielmeppiel March 12, 2026 21:17 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Initial plan

dd347cd

Copilot AI mentioned this pull request Mar 12, 2026

test: address PR review feedback on primitives discovery/parser tests #268

Merged

7 tasks

Copilot AI and others added 2 commits March 12, 2026 21:37

test: address PR review feedback on primitives discovery/parser tests

7efc881

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>

Merge pull request #268 from microsoft/copilot/sub-pr-254

110e270

test: address PR review feedback on primitives discovery/parser tests

danielmeppiel merged commit 4f30b3a into main Mar 12, 2026
6 checks passed

danielmeppiel deleted the test-assist/primitives-discovery-parser-coverage-da40935ebde67a0a branch March 12, 2026 21:53

This was referenced Mar 12, 2026

chore: update Unreleased changelog with PRs merged since v0.7.7 #271

Merged

[Test Improver] Monthly Activity 2026-03 #222

Closed

[Test Improver] Monthly Activity 2026-03 #309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Test Improver] test: add coverage tests for primitives discovery and parser#254

[Test Improver] test: add coverage tests for primitives discovery and parser#254
danielmeppiel merged 6 commits intomainfrom
test-assist/primitives-discovery-parser-coverage-da40935ebde67a0a

danielmeppiel commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

danielmeppiel commented Mar 12, 2026

Uh oh!

Copilot AI commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

danielmeppiel commented Mar 12, 2026

Goal

Approach

Covered paths

Coverage impact

Test status

Reproducibility

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

danielmeppiel commented Mar 12, 2026

Uh oh!

Copilot AI commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants