From 72592c3bca8f86f0058fb2368297501765619e76 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 10 Jun 2026 20:44:53 +0200 Subject: [PATCH] test: address claude-review on Pact consumer contract (#883) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pact.yml: pin tailscale/github-action@v3 to commit SHA (3 jobs) and pact-ruby-standalone install.sh to v2.6.1 (2 jobs) — supply-chain hardening - pact.yml: drop redundant `-o "addopts=..."` override (pyproject.toml already sets the same addopts; the override would silently mask future additions) - test_mcp_provider_verification.py: remove dead `pytestmark` shadowed by the list assignment; gate the module skip on PACT_USERNAME/PACT_PASSWORD too so a broker-set-but-creds-missing CI skips cleanly instead of raising KeyError - conftest.py: drop the unused `pact_dir` fixture Co-Authored-By: Claude Opus 4.8 --- .github/workflows/pact.yml | 14 +++++------ tests/contract/conftest.py | 6 ----- .../test_mcp_provider_verification.py | 25 +++++++++++-------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/.github/workflows/pact.yml b/.github/workflows/pact.yml index 228f8f8b..3c88ba4e 100644 --- a/.github/workflows/pact.yml +++ b/.github/workflows/pact.yml @@ -40,12 +40,12 @@ jobs: uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0 - name: Generate consumer pacts - run: uv run pytest -v -m contract -o "addopts=-p no:asyncio" tests/contract/ + run: uv run pytest -v -m contract tests/contract/ # Only publish from non-fork builds that have the broker secrets. - name: Join tailnet if: ${{ env.PACT_BROKER != '' }} - uses: tailscale/github-action@v3 + uses: tailscale/github-action@6cae46e2d796f265265cfcf628b72a32b4d7cade # v3 with: oauth-client-id: ${{ secrets.TS_OAUTH_CLIENT_ID }} oauth-secret: ${{ secrets.TS_OAUTH_SECRET }} @@ -54,7 +54,7 @@ jobs: - name: Install Pact CLI if: ${{ env.PACT_BROKER != '' }} run: | - curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/master/install.sh | bash + curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/v2.6.1/install.sh | bash echo "$PWD/pact/bin" >> "$GITHUB_PATH" - name: Publish pacts to broker @@ -110,7 +110,7 @@ jobs: - name: Join tailnet if: ${{ env.PACT_BROKER != '' }} - uses: tailscale/github-action@v3 + uses: tailscale/github-action@6cae46e2d796f265265cfcf628b72a32b4d7cade # v3 with: oauth-client-id: ${{ secrets.TS_OAUTH_CLIENT_ID }} oauth-secret: ${{ secrets.TS_OAUTH_SECRET }} @@ -124,7 +124,7 @@ jobs: PACT_PROVIDER_BRANCH: ${{ github.head_ref || github.ref_name }} # Publish results only from master so PR runs don't pollute the matrix. PACT_PUBLISH_RESULTS: ${{ github.ref == 'refs/heads/master' }} - run: uv run pytest -v -m contract -o "addopts=-p no:asyncio" tests/contract/test_mcp_provider_verification.py + run: uv run pytest -v -m contract tests/contract/test_mcp_provider_verification.py can-i-deploy: name: can-i-deploy @@ -133,7 +133,7 @@ jobs: if: ${{ github.ref == 'refs/heads/master' }} steps: - name: Join tailnet - uses: tailscale/github-action@v3 + uses: tailscale/github-action@6cae46e2d796f265265cfcf628b72a32b4d7cade # v3 with: oauth-client-id: ${{ secrets.TS_OAUTH_CLIENT_ID }} oauth-secret: ${{ secrets.TS_OAUTH_SECRET }} @@ -141,7 +141,7 @@ jobs: - name: Install Pact CLI run: | - curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/master/install.sh | bash + curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/v2.6.1/install.sh | bash echo "$PWD/pact/bin" >> "$GITHUB_PATH" - name: Can I deploy nextcloud-mcp-server? diff --git a/tests/contract/conftest.py b/tests/contract/conftest.py index 639875ce..2ab72ab3 100644 --- a/tests/contract/conftest.py +++ b/tests/contract/conftest.py @@ -30,12 +30,6 @@ def _clean_pact_dir(): yield -@pytest.fixture -def pact_dir() -> Path: - """Directory the generated pact files are written to.""" - return PACT_DIR - - @pytest.fixture def consumer_pact(): """A fresh Pact (consumer=nextcloud-mcp-server, provider=astrolabe). diff --git a/tests/contract/test_mcp_provider_verification.py b/tests/contract/test_mcp_provider_verification.py index 19bfd85e..a2bcc6ed 100644 --- a/tests/contract/test_mcp_provider_verification.py +++ b/tests/contract/test_mcp_provider_verification.py @@ -35,24 +35,30 @@ from collections.abc import Callable import pytest from pact import Verifier -pytestmark = pytest.mark.contract - logger = logging.getLogger(__name__) PROVIDER_NAME = "nextcloud-mcp-server" _PROVIDER_URL = os.environ.get("PACT_PROVIDER_URL") _BROKER_URL = os.environ.get("PACT_BROKER") +_BROKER_USERNAME = os.environ.get("PACT_USERNAME") +_BROKER_PASSWORD = os.environ.get("PACT_PASSWORD") _LOCAL_PACT_DIR = os.environ.get("PACT_PROVIDER_PACT_DIR") +# A usable broker source needs the URL *and* its basic-auth credentials; gating +# on all three keeps a misconfigured CI (broker set, creds missing) a clean skip +# rather than a confusing KeyError at verify time. +_BROKER_READY = bool(_BROKER_URL and _BROKER_USERNAME and _BROKER_PASSWORD) + # Skip the whole module unless we have a provider to hit AND a pact source. pytestmark = [ pytest.mark.contract, pytest.mark.skipif( - not _PROVIDER_URL or not (_BROKER_URL or _LOCAL_PACT_DIR), + not _PROVIDER_URL or not (_BROKER_READY or _LOCAL_PACT_DIR), reason=( - "Provider verification needs PACT_PROVIDER_URL and a pact source " - "(PACT_BROKER or PACT_PROVIDER_PACT_DIR). Skipped outside CI." + "Provider verification needs PACT_PROVIDER_URL and a pact source: " + "PACT_BROKER (+ PACT_USERNAME/PACT_PASSWORD) or " + "PACT_PROVIDER_PACT_DIR. Skipped outside CI." ), ), ] @@ -90,11 +96,10 @@ def test_verify_astrolabe_consumer_pacts() -> None: verifier = Verifier(PROVIDER_NAME).add_transport(url=_PROVIDER_URL) verifier.state_handler(_dispatch_state, teardown=True) - if _BROKER_URL: - # Basic-auth creds accompany the broker URL (see module skipif). - username = os.environ["PACT_USERNAME"] - password = os.environ["PACT_PASSWORD"] - verifier.broker_source(_BROKER_URL, username=username, password=password) + if _BROKER_URL and _BROKER_USERNAME and _BROKER_PASSWORD: + verifier.broker_source( + _BROKER_URL, username=_BROKER_USERNAME, password=_BROKER_PASSWORD + ) else: assert _LOCAL_PACT_DIR is not None # guaranteed by module skipif verifier.add_source(_LOCAL_PACT_DIR)