GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — refactor(tests): remove test smells in unit tests by victtorqueiroz · Pull Request #3451 · microsoft/apm

Conversation

Victor Queiroz (victtorqueiroz) commented Nov 2, 2026

Copy link

Copy Markdown

Description

What this PR does

Refactors 40 test-smell occurrences detected by the TEMPY tool in tests/unit/, one commit per occurrence:

Test smell	Occurrences	Main technique
Exception Handling	10	`pytest.raises` instead of manual `try/except` + `fail`; `tmp_path` + `monkeypatch.chdir` instead of `try/finally` + `os.chdir`
Verbose Test	10	Extract long setup into helper functions; remove duplicated literals
Unknown Test	10	Add the missing assertion on the returned value, captured output or mock calls
Conditional Test Logic	10	`pytest.mark.parametrize` and small helpers instead of loops/conditionals inside tests

(41 commits in total: one extra commit reuses a helper introduced by the previous one.)

Why

Test smells make tests harder to read and maintain, and some hide real problems: a manual try/except can pass without the exception ever being raised, and a test without assertions passes as long as nothing crashes. The refactorings follow the CATSPY catalog of Python test smells.

How it was tested

Ran pytest on all 21 changed files on top of the current main: 1579 passed.
3 tests in TestCoworkSkillsDirFunctions (test_config_command.py) fail on Windows because they use POSIX paths (/absolute/...). They are untouched by this PR and fail the same way without it.
Re-ran TEMPY on each changed file to confirm the smell was removed.

Notes for the reviewer

Only files under tests/ are changed; no production code.
Assertions were kept as they were, except in the Unknown Test refactorings, where an assertion was added.
Each commit is self-contained, so the PR can be reviewed commit by commit. If it is too large for one review, we can split it into one PR per smell type.
Some commit messages are in Portuguese because of our course's commit format; we can reword them if needed.
This work was done for a university course (Software Quality, UFC, Brazil), with the help of Claude Code (AI coding assistant) for explanations, review and git workflow.

Issue and approved scope

Issue: #3450

This is an unsolicited contribution proposed in #3450.

Does this PR complete the issue, or what remains? It completes the issue.

Type of change

Bug fix
New feature
Documentation
Maintenance / refactor

Testing

Tested locally
All existing tests pass (except 3 pre-existing Windows-only failures, see above)
Added tests for new functionality (if applicable)

Spec conformance (OpenAPM v0.1)

N/A -- this PR does not change OpenAPM-observable behaviour.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

radleidoroth and others added 30 commits

November 2, 2026 16:55


          test: assert return value in test_all_defaults_no_error (Unknown Test)

5c5c0c8


          test: assert return value in test_mcp_name_with_transport_stdio_no_ur…

743ec2e

…l (Unknown Test)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert return value in test_mcp_name_with_command_argv (Unknown…

cfcea76

… Test)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert return value in test_headers_with_url (Unknown Test)

a85d8c4

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert _lookup_lockfile_ref is not called in test_no_project_ro…

7e5ecd0

…ot_skips_lockfile (Unknown Test)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert captured output in test_rich_rendering_no_context_files

a42b0f3

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert captured fallback output in test_import_error_fallback_text

8adf903

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert missing dir is not created in test_write_marker_silently…

8471ef6

…_skips_missing_dir (Unknown Test)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert cache stays empty in test_noop_for_unknown_server (Unkno…

66b4847

…wn Test)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: assert _rich_echo is not called in test_empty_host_is_noop (Unk…

fab00a3

…nown Test)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: extract basic-auth header helper in test_ado_git_env_strips_raw…

4ef1633

…_github_token_sources (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: extract basic-auth header helper in test_gitlab_git_env_uses_sa…

e2459c1

…fe_basic_header (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: use pool.map instead of future loop in test_concurrent_same_hos…

c67e039

…t_emits_once (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: parametrize cases and extract accessible-instructions helper in…

d29a69f

… test_hierarchical_coverage_verification (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: parametrize files and extract coverage helper in test_coverage_…

…constraint_priority_over_efficiency (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: parametrize single-target primitives in test_skills_is_multi_ta…

6eefecb

…rget (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: parametrize dispatch entries in test_dispatch_entries_have_vali…

334937b

…d_methods (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: parametrize targets and write files explicitly in test_filter_g…

528f683

…eneric_hooks_universal (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: extract credential-free environment helper in test_private_gith…

d6c484c

…ub_file_fetch_uses_one_scoped_fallback (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: reuse credential-free environment helper in test_private_github…

bc0ccbf

…_clone_resolves_one_path_scoped_fallback (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          test: parametrize (target, primitive) pairs in test_every_target_prim…

3b6e889

…itive_has_dispatch_path (Conditional Test Logic)

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>


          refactor: remove exception handling em tests/unit/test_mcp_overlays.py

2433afb


          refactor: remove exception handling em tests/unit/compilation/test_co…

03df7ff

…mpilation.py


          refactor: remove exception handling em tests/unit/test_build_spec.py

b82d695


          refactor: remove exception handling em tests/unit/test_runtime_detect…

54b6c33

…ion.py


          refactor: remove exception handling em tests/unit/test_config_command.py

b7ca47c


          refactor: remove exception handling em tests/unit/test_script_runner.…

26ff56c

…py (test_resolve_prompt_file_local_exists)


          refactor: remove exception handling em tests/unit/test_script_runner.…

…py (test_resolve_prompt_file_dependency_root)


          refactor: remove exception handling em tests/unit/test_script_runner.…

d166291

…py (test_resolve_prompt_file_dependency_subdirectory)


          refactor: remove exception handling em tests/unit/test_script_runner.…

c449521

…py (test_resolve_prompt_file_multiple_dependencies)

Victor Queiroz (victtorqueiroz) added 11 commits

November 2, 2026 17:01


          refactor: remove exception handling em tests/unit/test_script_runner.…

e19fca9

…py (test_resolve_prompt_file_no_apm_modules)


          refactor: remove verbose test em tests/unit/integration/test_skill_in…

a8faabc

…tegrator_hermetic.py


          refactor: remove verbose test em tests/unit/integration/test_skill_in…

aab9384

…tegrator_phase3w4.py


          refactor: remove verbose test em tests/unit/integration/test_skill_in…

a436345

…tegrator.py (test_native_skill_preserves_complete_structure)


          refactor: remove verbose test em tests/unit/integration/test_skill_in…

c70f751

…tegrator.py (test_force_overwrites_identical_bin)


          refactor: remove verbose test em tests/unit/deps/test_shared_clone_ca…

9d29aee

…che.py (test_short_sha_skips_tier1_and_resolves_via_tier2)


          refactor: remove verbose test em tests/unit/integration/test_hook_int…

cbe2025

…egrator.py (test_stale_sidecar_removed_on_sync)


          refactor: remove verbose test em tests/unit/integration/test_hook_int…

ce2f634

…egrator_defect_regression.py


          refactor: remove verbose test em tests/unit/deps/test_shared_clone_ca…

b0edcea

…che.py (test_bare_action_rmtrees_target_before_init)


          refactor: remove verbose test em tests/unit/deps/test_shared_clone_ca…

84db8b5

…che.py (test_fetch_action_uses_explicit_url_not_origin)


          refactor: remove verbose test em tests/unit/integration/test_hook_int…

1bd01d6

…egrator.py (test_root_local_healer_preserves_dependency_source_entries)

Copilot AI balanced review requested due to automatic review settings

November 2, 2026 21:17

Victor Queiroz (victtorqueiroz) requested review from Daniel Meppiel (danielmeppiel) and Sergio Sisternes (sergio-sisternes-epam) as code owners

November 2, 2026 21:17

Copilot started reviewing on behalf of Victor Queiroz (victtorqueiroz)

November 2, 2026 21:18

View session

Victor Queiroz (victtorqueiroz) mentioned this pull request

Nov 2, 2026

[TEST] Refactor test smells in unit tests #3450

Open

Copilot AI reviewed

Nov 2, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🟡 Changes recommended

The changes introduce an import-order violation and whitespace on blank lines that fail the required Ruff lint and format checks.

2 open issues

Remove trailing spaces from blank lines · New
Fix import ordering to satisfy Ruff · New

What changed in this PR

This test-only refactor addresses the 40 test-smell occurrences proposed in #3450 across 21 files. It simplifies APM's test setup and strengthens selected assertions without changing production code.

Changes:

Replace manual exception handling and directory restoration with direct calls, pytest.raises, and pytest fixtures.
Extract setup helpers and parameterize repeated test cases.
Add assertions for output, mock calls, return values, and filesystem state.

File	Description
`tests/unit/test_view_command_cli_surface.py`	Assert rendered output and skipped lockfile lookup.
`tests/unit/test_script_runner.py`	Use fixtures for temporary directories and working-directory restoration.
`tests/unit/test_runtime_detection.py`	Let unexpected client-creation exceptions fail directly.
`tests/unit/test_mcp_overlays.py`	Use `pytest.raises` for invalid command validation.
`tests/unit/test_config_command.py`	Simplify outside-project setup with fixtures.
`tests/unit/test_build_spec.py`	Let compilation errors fail directly.
`tests/unit/test_auth.py`	Extract header filtering and simplify concurrent calls.
`tests/unit/integration/test_skill_integrator.py`	Extract complete-skill and preinstalled-plugin setup.
`tests/unit/integration/test_skill_integrator_phase3w4.py`	Extract skill-collision setup.
`tests/unit/integration/test_skill_integrator_hermetic.py`	Extract hermetic skill-collision setup.
`tests/unit/integration/test_mcp_integrator.py`	Assert unknown-server overlays leave the cache empty.
`tests/unit/integration/test_hook_integrator.py`	Reuse hook data and parameterize target checks.
`tests/unit/integration/test_hook_integrator_defect_regression.py`	Extract legacy hook-migration package setup.
`tests/unit/integration/test_data_driven_dispatch.py`	Parameterize dispatch completeness and method checks.
`tests/unit/install/test_mcp_conflicts.py`	Assert successful validation returns `None`.
`tests/unit/install/test_cache_pin.py`	Assert marker writing does not create missing directories.
`tests/unit/deps/test_shared_clone_cache.py`	Extract downloader and mocked fetch setup.
`tests/unit/core/test_public_github_anonymous_first.py`	Reuse credential-free environment construction.
`tests/unit/core/test_auth_phase3.py`	Assert an empty host produces no diagnostic output.
`tests/unit/compilation/test_coverage_guarantees.py`	Extract coverage helpers and parameterize file cases.
`tests/unit/compilation/test_compilation.py`	Let validation-display exceptions fail directly.

_{Review effort: Balanced}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/unit/compilation/test_compilation.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

tests/unit/integration/test_data_driven_dispatch.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          style(tests): fix ruff findings (import order, unused import, blank-l…

aed209b

…ine whitespace)

Victor Queiroz (victtorqueiroz) commented Nov 2, 2026

Copy link

Copy Markdown

Author

@microsoft-github-policy-service agree

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sergio Sisternes (sergio-sisternes-epam) commented Nov 3, 2026

Copy link

Copy Markdown

Collaborator

Thank you for proposing this bounded test-maintenance contribution. APM starts with an issue, not an implementation (https://github.com/microsoft/apm/blob/main/CONTRIBUTING.md). The linked issue #3450 is open but not marked status/accepted; please ask a maintainer to review and accept the scope there before implementation proceeds. This PR is labelled status/deferred until then. Current review requests include Daniel Meppiel (@danielmeppiel) and Sergio Sisternes (@sergio-sisternes-epam).

Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sergio Sisternes (sergio-sisternes-epam) added area/testing

Test infrastructure, fixtures, e2e harness, coverage.

status/deferred

Not invited for implementation now; no release commitment.

triage/recommended Automated advice completed; not human scope approval. labels

Nov 3, 2026

This branch has not been deployed

No deployments

Reviewers

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

Daniel Meppiel (danielmeppiel)

Awaiting requested review from danielmeppiel

danielmeppiel is a code owner

Sergio Sisternes (sergio-sisternes-epam)

Awaiting requested review from sergio-sisternes-epam

sergio-sisternes-epam is a code owner

At least 0 approving reviews are required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

area/testing

Test infrastructure, fixtures, e2e harness, coverage.

status/deferred

Not invited for implementation now; no release commitment.

triage/recommended

Automated advice completed; not human scope approval.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.