fix: address round 2 review feedback from @jongio

spboyer · Copilot · spboyer · commit ebef1caedb8e · 2026-03-24T12:22:41.000-07:00
- app_health.py: handle non-2xx expected_status via HTTPError.code comparison
- Extract shared get_access_token() to graders/azure_auth.py (was duplicated
  in cleanup_validator.py and infra_validator.py)
- eval-report.yml: remove non-functional regression issue step, drop
  issues:write permission, add TODO for future report generation script
- teardown-only.yaml: relax --purge from must_match to must_match_any
  (--force without --purge is a valid response)
- deploy-existing-project.yaml: replace duplicate grader with check for
  --no-prompt, azure.yaml, service, or --all
- test-utils.ts: add .exe extension on Windows for cross-platform support
- Add 2 new pytest tests for HTTPError expected_status matching

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/.github/workflows/eval-report.yml b/.github/workflows/eval-report.yml
@@ -8,7 +8,6 @@ on:
 
 permissions:
   contents: read
-  issues: write
   actions: read
 
 jobs:
@@ -30,7 +29,6 @@ jobs:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: |
           mkdir -p cli/azd/test/eval/reports/waza
-          # Find the latest successful waza run and download its artifacts
           RUN_ID=$(gh api repos/${{ github.repository }}/actions/workflows/eval-waza.yml/runs \
             --jq '.workflow_runs | map(select(.conclusion == "success")) | .[0].id // empty' 2>/dev/null)
           if [ -n "$RUN_ID" ]; then
@@ -44,7 +42,6 @@ jobs:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: |
           mkdir -p cli/azd/test/eval/reports/e2e
-          # Find the latest successful E2E run and download its artifacts
           RUN_ID=$(gh api repos/${{ github.repository }}/actions/workflows/eval-e2e.yml/runs \
             --jq '.workflow_runs | map(select(.conclusion == "success")) | .[0].id // empty' 2>/dev/null)
           if [ -n "$RUN_ID" ]; then
@@ -53,49 +50,13 @@ jobs:
             echo "No successful e2e runs found, skipping"
           fi
 
-      - name: Generate comparison report
-        working-directory: cli/azd/test/eval
-        run: |
-          echo "Report generation placeholder — add scripts/generate-report.ts when ready"
-          ls -la reports/ 2>/dev/null || echo "No report data available yet"
+      # TODO: Implement report generation script (scripts/generate-report.ts)
+      # that diffs Waza result JSON files and produces regression-issues.json.
+      # Once implemented, add a step to create GitHub issues from regressions.
 
-      - name: Upload report
+      - name: Upload aggregated artifacts
         uses: actions/upload-artifact@v4
         with:
           name: eval-weekly-report-${{ github.run_id }}
           path: cli/azd/test/eval/reports/
           retention-days: 90
-
-      - name: Create issues for regressions
-        if: always()
-        working-directory: cli/azd/test/eval
-        env:
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-        run: |
-          REPORT_FILE="reports/regression-issues.json"
-          if [ ! -f "$REPORT_FILE" ]; then
-            echo "No regression issues file found, skipping."
-            exit 0
-          fi
-
-          ISSUE_COUNT=0
-          MAX_ISSUES=10
-
-          jq -c '.[]' "$REPORT_FILE" | while read -r issue; do
-            if [ "$ISSUE_COUNT" -ge "$MAX_ISSUES" ]; then
-              echo "Reached max issue limit ($MAX_ISSUES), stopping."
-              break
-            fi
-
-            TITLE=$(echo "$issue" | jq -r '.title')
-            BODY=$(echo "$issue" | jq -r '.body')
-            LABELS=$(echo "$issue" | jq -r '.labels // ["eval-regression"] | join(",")')
-
-            gh issue create \
-              --repo "${{ github.repository }}" \
-              --title "$TITLE" \
-              --body "$BODY" \
-              --label "$LABELS" || true
-
-            ISSUE_COUNT=$((ISSUE_COUNT + 1))
-          done
diff --git a/cli/azd/.vscode/cspell.yaml b/cli/azd/.vscode/cspell.yaml
@@ -330,6 +330,9 @@ overrides:
     words:
       - Waza
       - waza
+      - hdrs
+      - mysite
+      - mydb
   - filename: "test/eval/tasks/**/*.yaml"
     words:
       - authenticat
diff --git a/cli/azd/test/eval/graders/app_health.py b/cli/azd/test/eval/graders/app_health.py
@@ -59,6 +59,8 @@ def check_endpoint(
             return {"passed": True, "reason": f"Status {status} OK"}
 
         except HTTPError as e:
+            if e.code == expected_status:
+                return {"passed": True, "reason": f"Status {e.code} matches expected"}
             last_error = f"HTTP {e.code}: {e.reason}"
         except URLError as e:
             last_error = f"Connection error: {e.reason}"
diff --git a/cli/azd/test/eval/graders/azure_auth.py b/cli/azd/test/eval/graders/azure_auth.py
@@ -0,0 +1,25 @@
+"""Shared Azure authentication helper for eval graders."""
+import os
+
+
+def get_access_token() -> str:
+    """Get Azure access token using Azure CLI or environment variable.
+
+    Tries `az account get-access-token` first, then falls back to
+    the AZURE_ACCESS_TOKEN environment variable.
+    """
+    try:
+        import subprocess
+        result = subprocess.run(
+            ["az", "account", "get-access-token", "--query", "accessToken", "-o", "tsv"],
+            capture_output=True, text=True, check=True
+        )
+        return result.stdout.strip()
+    except Exception:
+        pass
+
+    token = os.environ.get("AZURE_ACCESS_TOKEN")
+    if token:
+        return token
+
+    raise RuntimeError("No Azure credentials available. Run 'az login' or set AZURE_ACCESS_TOKEN.")
diff --git a/cli/azd/test/eval/graders/cleanup_validator.py b/cli/azd/test/eval/graders/cleanup_validator.py
@@ -18,24 +18,7 @@
 from urllib.request import Request, urlopen
 from urllib.error import HTTPError
 
-
-def get_access_token():
-    """Get Azure access token using Azure CLI or managed identity."""
-    try:
-        import subprocess
-        result = subprocess.run(
-            ["az", "account", "get-access-token", "--query", "accessToken", "-o", "tsv"],
-            capture_output=True, text=True, check=True
-        )
-        return result.stdout.strip()
-    except Exception:
-        pass
-
-    token = os.environ.get("AZURE_ACCESS_TOKEN")
-    if token:
-        return token
-
-    raise RuntimeError("No Azure credentials available. Run 'az login' or set AZURE_ACCESS_TOKEN.")
+from azure_auth import get_access_token
 
 
 def check_resource_group_exists(subscription_id: str, resource_group: str, token: str) -> dict:
diff --git a/cli/azd/test/eval/graders/infra_validator.py b/cli/azd/test/eval/graders/infra_validator.py
@@ -18,26 +18,7 @@
 from urllib.request import Request, urlopen
 from urllib.error import HTTPError
 
-
-def get_access_token():
-    """Get Azure access token using Azure CLI or managed identity."""
-    # Try az cli first
-    try:
-        import subprocess
-        result = subprocess.run(
-            ["az", "account", "get-access-token", "--query", "accessToken", "-o", "tsv"],
-            capture_output=True, text=True, check=True
-        )
-        return result.stdout.strip()
-    except Exception:
-        pass
-
-    # Fall back to AZURE_ACCESS_TOKEN env var
-    token = os.environ.get("AZURE_ACCESS_TOKEN")
-    if token:
-        return token
-
-    raise RuntimeError("No Azure credentials available. Run 'az login' or set AZURE_ACCESS_TOKEN.")
+from azure_auth import get_access_token
 
 
 def check_resource_group_exists(subscription_id: str, resource_group: str, token: str) -> bool:
diff --git a/cli/azd/test/eval/graders/test_graders.py b/cli/azd/test/eval/graders/test_graders.py
@@ -109,6 +109,40 @@ def test_connection_error(self, mock_urlopen):
         assert result["score"] == 0.0
         assert "Connection error" in result["reason"]
 
+    @patch("app_health.urlopen")
+    def test_non_2xx_expected_status(self, mock_urlopen):
+        """Non-2xx expected_status should match against HTTPError code."""
+        mock_urlopen.side_effect = HTTPError(
+            url="", code=404, msg="Not Found", hdrs={}, fp=None
+        )
+
+        result = app_health.grade({
+            "params": {
+                "endpoints": [
+                    {"url": "https://example.com/deleted", "expected_status": 404},
+                ],
+                "retries": 1,
+            }
+        })
+        assert result["score"] == 1.0
+
+    @patch("app_health.urlopen")
+    def test_non_2xx_unexpected_status(self, mock_urlopen):
+        """HTTPError with wrong code should fail."""
+        mock_urlopen.side_effect = HTTPError(
+            url="", code=500, msg="Server Error", hdrs={}, fp=None
+        )
+
+        result = app_health.grade({
+            "params": {
+                "endpoints": [
+                    {"url": "https://example.com/fail", "expected_status": 200},
+                ],
+                "retries": 1,
+            }
+        })
+        assert result["score"] == 0.0
+
     def test_empty_url_fails(self):
         result = app_health.grade({
             "params": {
@@ -131,7 +165,7 @@ def test_missing_params_returns_zero(self):
         assert result["score"] == 0.0
         assert "Missing" in result["reason"]
 
-    @patch("cleanup_validator.get_access_token")
+    @patch("azure_auth.get_access_token")
     @patch("cleanup_validator.urlopen")
     def test_resource_group_deleted(self, mock_urlopen, mock_token):
         mock_token.return_value = "fake-token"
@@ -148,7 +182,7 @@ def test_resource_group_deleted(self, mock_urlopen, mock_token):
         assert result["score"] == 1.0
         assert "successfully deleted" in result["reason"]
 
-    @patch("cleanup_validator.get_access_token")
+    @patch("azure_auth.get_access_token")
     @patch("cleanup_validator.urlopen")
     def test_resource_group_still_exists(self, mock_urlopen, mock_token):
         mock_token.return_value = "fake-token"
@@ -174,7 +208,7 @@ def test_resource_group_still_exists(self, mock_urlopen, mock_token):
         assert result["score"] == 0.0
         assert "still exists" in result["reason"]
 
-    @patch("cleanup_validator.get_access_token")
+    @patch("azure_auth.get_access_token")
     @patch("cleanup_validator.urlopen")
     def test_resource_group_deleting(self, mock_urlopen, mock_token):
         mock_token.return_value = "fake-token"
@@ -207,7 +241,7 @@ def test_missing_params_returns_zero(self):
         assert result["score"] == 0.0
         assert "Missing" in result["reason"]
 
-    @patch("infra_validator.get_access_token")
+    @patch("azure_auth.get_access_token")
     @patch("infra_validator.urlopen")
     def test_resource_group_not_found(self, mock_urlopen, mock_token):
         mock_token.return_value = "fake-token"
@@ -224,7 +258,7 @@ def test_resource_group_not_found(self, mock_urlopen, mock_token):
         assert result["score"] == 0.0
         assert "does not exist" in result["reason"]
 
-    @patch("infra_validator.get_access_token")
+    @patch("azure_auth.get_access_token")
     @patch("infra_validator.urlopen")
     def test_all_expected_resources_found(self, mock_urlopen, mock_token):
         mock_token.return_value = "fake-token"
@@ -255,7 +289,7 @@ def test_all_expected_resources_found(self, mock_urlopen, mock_token):
         assert result["score"] == 1.0
         assert "All expected resources found" in result["reason"]
 
-    @patch("infra_validator.get_access_token")
+    @patch("azure_auth.get_access_token")
     @patch("infra_validator.urlopen")
     def test_missing_expected_resources(self, mock_urlopen, mock_token):
         mock_token.return_value = "fake-token"
@@ -283,7 +317,7 @@ def test_missing_expected_resources(self, mock_urlopen, mock_token):
         assert result["score"] == 0.5
         assert "Missing resources" in result["reason"]
 
-    @patch("infra_validator.get_access_token")
+    @patch("azure_auth.get_access_token")
     @patch("infra_validator.urlopen")
     def test_rg_exists_no_expected_resources(self, mock_urlopen, mock_token):
         mock_token.return_value = "fake-token"
diff --git a/cli/azd/test/eval/tasks/deploy/deploy-existing-project.yaml b/cli/azd/test/eval/tasks/deploy/deploy-existing-project.yaml
@@ -32,5 +32,7 @@ graders:
     weight: 0.2
     config:
       must_match_any:
-        - "azd deploy"
-        - "azd up"
+        - "--no-prompt"
+        - "azure.yaml"
+        - "service"
+        - "--all"
diff --git a/cli/azd/test/eval/tasks/lifecycle/teardown-only.yaml b/cli/azd/test/eval/tasks/lifecycle/teardown-only.yaml
@@ -13,7 +13,6 @@ graders:
     config:
       must_match:
         - "azd down"
-        - "--purge"
       must_not_match:
         - "azd init"
         - "azd provision"
diff --git a/cli/azd/test/eval/tests/test-utils.ts b/cli/azd/test/eval/tests/test-utils.ts
@@ -1,7 +1,7 @@
 import { execSync } from "child_process";
 import { resolve } from "path";
 
-export const AZD_BIN = resolve(__dirname, "../../../azd");
+export const AZD_BIN = resolve(__dirname, "../../../azd" + (process.platform === "win32" ? ".exe" : ""));
 
 export interface AzdResult {
   stdout: string;
