Skip to content

fix(tests): make doctor tests deterministic across CI environments#997

Merged
sergio-sisternes-epam merged 1 commit intomainfrom
fix/doctor-test-determinism
Apr 27, 2026
Merged

fix(tests): make doctor tests deterministic across CI environments#997
sergio-sisternes-epam merged 1 commit intomainfrom
fix/doctor-test-determinism

Conversation

@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

Problem

The 6 test_marketplace_doctor.py tests have been failing on Linux and Windows CI while passing on macOS, breaking the main branch CI pipeline.

Root Cause

Two interacting bugs:

  1. Stale mock entry: The mock side_effect lists included _GIT_CRED_OK for a git credential fill subprocess call. Whether this call actually happens depends on whether AuthResolver finds a token from environment variables first. On Linux/Windows CI, GITHUB_APM_PAT is set, so AuthResolver skips the git credential fallback — only 3 subprocess.run calls instead of 4. The positional mock shifts, and gh --version gets the wrong mock response.

  2. Incomplete env cleanup: test_no_token_informational cleared GITHUB_TOKEN and GH_TOKEN but not GITHUB_APM_PAT. On CI (where GITHUB_APM_PAT is set), AuthResolver still found a token, so the "unauthenticated" assertion failed.

Fix

  • Autouse fixture that mocks AuthResolver and clears all token env vars (GITHUB_APM_PAT, GITHUB_TOKEN, GH_TOKEN), making auth behaviour deterministic regardless of host environment.
  • Removed _GIT_CRED_OK from all 28 mock side_effect lists (3 subprocess.run calls from marketplace.py, not 4).
  • Override the AuthResolver mock with token=None in the no-token test.

Verification

  • All 33 doctor tests pass locally with and without GITHUB_APM_PAT/GITHUB_TOKEN env vars set
  • Full unit suite (6564 tests) passes with zero failures

The 6 marketplace doctor tests failed on Linux/Windows CI but passed on
macOS because:

1. CI sets GITHUB_APM_PAT env var on Linux/Windows but not macOS.
   When present, AuthResolver finds the token from env and skips the
   git-credential-fill subprocess call, reducing subprocess.run calls
   from 4 to 3.  The positional mock side_effects then shift -- gh
   --version consumes the stale _GIT_CRED_OK entry (empty stdout)
   instead of _GH_OK.

2. test_no_token_informational cleared GITHUB_TOKEN and GH_TOKEN but
   not GITHUB_APM_PAT, so AuthResolver still found a token on CI.

Fix:
- Add autouse fixture that mocks AuthResolver and clears all token env
  vars, making auth behaviour deterministic regardless of host env.
- Remove the stale _GIT_CRED_OK mock entry from all 28 side_effect
  lists (3 subprocess.run calls, not 4).
- Override the AuthResolver mock with token=None in the no-token test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR makes the apm marketplace doctor unit tests deterministic across CI environments by removing dependence on host-provided auth environment variables and by stabilizing the expected subprocess.run call sequence.

Changes:

  • Added an autouse fixture that clears common token env vars and mocks AuthResolver to return a deterministic auth context.
  • Removed the stale git credential fill mock entry (_GIT_CRED_OK) from subprocess.run.side_effect lists to match the actual call pattern under test.
  • Updated the “no token” test to override the autouse AuthResolver mock with token=None.
Comments suppressed due to low confidence (2)

tests/unit/commands/test_marketplace_doctor.py:226

  • test_github_token_detected sets GITHUB_TOKEN and asserts that token value is not printed, but the new autouse _mock_auth_resolver fixture forces AuthResolver.resolve(...).token to always be "mock-doctor-token". That means this test no longer verifies either env-var detection or token redaction (it would still pass even if the real token were printed). Consider overriding the AuthResolver mock in this test to return the same token value you assert on (or assert that the mocked token is not present and drop the env-var setup/rename the test).
    def test_github_token_detected(self, mock_run, runner, tmp_path, monkeypatch):
        monkeypatch.chdir(tmp_path)
        monkeypatch.setenv("GITHUB_TOKEN", "ghp_test123")
        mock_run.side_effect = [
            _make_run_result(0, stdout="git version 2.40.0"),
            _make_run_result(0),
            _GH_OK,
        ]

        result = runner.invoke(marketplace, ["doctor"])
        assert "Token detected" in result.output
        # Must NOT print the actual token
        assert "ghp_test123" not in result.output

tests/unit/commands/test_marketplace_doctor.py:241

  • Same issue as the GITHUB_TOKEN case: test_gh_token_detected sets GH_TOKEN and asserts that "gho_test456" is not printed, but the autouse AuthResolver mock always returns "mock-doctor-token", so this assertion no longer exercises token redaction and the env-var manipulation is effectively dead code. Override the AuthResolver mock here to return "gho_test456" (or assert on the mocked token and simplify/rename the test).
    def test_gh_token_detected(self, mock_run, runner, tmp_path, monkeypatch):
        monkeypatch.chdir(tmp_path)
        monkeypatch.delenv("GITHUB_TOKEN", raising=False)
        monkeypatch.setenv("GH_TOKEN", "gho_test456")
        mock_run.side_effect = [
            _make_run_result(0, stdout="git version 2.40.0"),
            _make_run_result(0),
            _GH_OK,
        ]

        result = runner.invoke(marketplace, ["doctor"])
        assert "Token detected" in result.output
        assert "gho_test456" not in result.output

Comment on lines +40 to +42
# Token env vars that AuthResolver inspects. Cleared in the autouse
# fixture below so doctor tests are deterministic regardless of CI env.
_TOKEN_ENV_VARS = ("GITHUB_APM_PAT", "GITHUB_TOKEN", "GH_TOKEN")
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

_TOKEN_ENV_VARS/comment implies these are the token env vars AuthResolver inspects, but AuthResolver also checks per-org vars like GITHUB_APM_PAT_<ORG> before the global ones. If the intent is "clear all auth env vars" for determinism, consider also clearing any env vars with the GITHUB_APM_PAT_ prefix (or reword the comment/constant name to reflect that this only targets the global vars).

Copilot uses AI. Check for mistakes.
@sergio-sisternes-epam sergio-sisternes-epam added this pull request to the merge queue Apr 27, 2026
Merged via the queue into main with commit 76cd095 Apr 27, 2026
19 checks passed
@sergio-sisternes-epam sergio-sisternes-epam deleted the fix/doctor-test-determinism branch April 27, 2026 17:58
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