From 46c64997089186e335b08d1f6194d3ed5628f1a5 Mon Sep 17 00:00:00 2001 From: Ian Flores Siaca <18703558+ian-flores@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:21:24 -0700 Subject: [PATCH 1/2] test: classify every skip in eight files as unproven or not-applicable Second slice of #616. The unproven outcome shipped in #617 but nothing used it, so the distinction existed without changing what any real run reported. Triage eight files completely: 21 skips become attest.unproven, 32 become attest.not_applicable. The unproven ones share a shape -- VIP reached the thing it was asked to check and could not complete the check. An unreachable host in the TLS suite, an unreachable git repo, a version endpoint that returned no version, six Workbench UI controls that never appeared, a Kubernetes client that could not be built against a configured cluster, a session that never returned from resume. Each of those was previously indistinguishable from "this deployment does not have that feature". Add selftests/test_skip_triage.py, which fails when a bare pytest.skip appears in a file listed as triaged. Without it the next skip added to one of these files silently reintroduces the ambiguity, and the triage decays back to where it started. --- AGENTS.md | 1 + selftests/test_skip_triage.py | 61 +++++++++++++++++++ src/vip_tests/connect/test_content_deploy.py | 27 ++++---- src/vip_tests/cross_product/test_ssl.py | 23 +++---- src/vip_tests/prerequisites/test_versions.py | 15 ++--- src/vip_tests/workbench/test_ide_launch.py | 11 ++-- src/vip_tests/workbench/test_jobs.py | 19 +++--- .../workbench/test_session_capacity.py | 7 ++- .../workbench/test_session_capacity_k8s.py | 21 ++++--- src/vip_tests/workbench/test_sessions.py | 3 +- 10 files changed, 133 insertions(+), 55 deletions(-) create mode 100644 selftests/test_skip_triage.py diff --git a/AGENTS.md b/AGENTS.md index d3f5eca9..506b9c61 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -389,6 +389,7 @@ Register warning filters in `src/vip/plugin.py::pytest_configure` (via `config.a - Adding a Workbench scenario that ends the shared auth session (sign-out, session revocation, password change) without ordering it last *and* restoring the session afterwards. Under `--interactive-auth` / `--headless-auth` every Workbench scenario shares one account, so ending that session breaks every scenario still running on other xdist workers, plus the cached auth session on disk. `test_workbench_signout` is the worked example. - Creating `.py` step files without a matching `.feature` file (or vice versa). - Forgetting the `@connect`/`@workbench`/`@package_manager` tag in feature files (breaks auto-skip). +- Adding a bare `pytest.skip()` to a file listed in `selftests/test_skip_triage.py`. Every skip in those files has been deliberately classified, and `test_skip_triage.py` fails the build if a new unclassified one appears — use `attest.unproven()` or `attest.not_applicable()`. - Reaching for a bare `pytest.skip()` when the real situation is "I could not check this". That is the failure mode #616 exists to close: an unverified deployment reporting itself as a passing one. If the product was configured and you still could not run the check, use `vip.attest.unproven()`. - Using non-conventional PR titles (must be `type: description`). - Relying on multi-line formatting to shorten lines -- `ruff format` will collapse list comprehensions back to one line if they fit within 100 chars. Extract a helper function instead. diff --git a/selftests/test_skip_triage.py b/selftests/test_skip_triage.py new file mode 100644 index 00000000..3374c792 --- /dev/null +++ b/selftests/test_skip_triage.py @@ -0,0 +1,61 @@ +"""Guards the skip-classification triage (#616). + +A bare ``pytest.skip()`` still means "not applicable", which is the right +default for the sites nobody has looked at yet. But once a file *has* been +triaged, every skip in it should say which kind it is out loud -- otherwise +the next person to add a skip there silently reintroduces the ambiguity the +triage just removed, and the file quietly drifts back. + +Add a file to ``TRIAGED_FILES`` when every skip in it has been deliberately +classified as ``attest.unproven`` or ``attest.not_applicable``. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +_SRC = Path(__file__).resolve().parent.parent / "src" / "vip_tests" + +# Files whose every skip has been deliberately classified. Growing this list +# is the point; see #616. +TRIAGED_FILES = [ + "connect/test_content_deploy.py", + "cross_product/test_ssl.py", + "prerequisites/test_versions.py", + "workbench/test_ide_launch.py", + "workbench/test_jobs.py", + "workbench/test_session_capacity.py", + "workbench/test_session_capacity_k8s.py", + "workbench/test_sessions.py", +] + +_BARE_SKIP = re.compile(r"\bpytest\.skip\s*\(") + + +@pytest.mark.parametrize("relpath", TRIAGED_FILES) +def test_triaged_file_has_no_unclassified_skip(relpath: str): + path = _SRC / relpath + assert path.exists(), f"{relpath} moved or was deleted; update TRIAGED_FILES" + offenders = [ + f"{relpath}:{n}" + for n, line in enumerate(path.read_text().splitlines(), 1) + if _BARE_SKIP.search(line) + ] + assert not offenders, ( + "bare pytest.skip() in a triaged file -- say which kind of skip this is " + "with attest.unproven() or attest.not_applicable():\n " + "\n ".join(offenders) + ) + + +@pytest.mark.parametrize("relpath", TRIAGED_FILES) +def test_triaged_file_actually_uses_the_helpers(relpath: str): + """Guards against 'triaging' a file by deleting its skips.""" + text = (_SRC / relpath).read_text() + assert "attest" in text, f"{relpath} is listed as triaged but never imports attest" + + +def test_triage_list_has_no_duplicates(): + assert len(TRIAGED_FILES) == len(set(TRIAGED_FILES)) diff --git a/src/vip_tests/connect/test_content_deploy.py b/src/vip_tests/connect/test_content_deploy.py index 3719993c..ab1831c3 100644 --- a/src/vip_tests/connect/test_content_deploy.py +++ b/src/vip_tests/connect/test_content_deploy.py @@ -15,6 +15,7 @@ import pytest from pytest_bdd import scenario, then, when +from vip import attest from vip_tests.connect.bundles import _latest_version, build_shiny_bundle_files from vip_tests.connect.conftest import _make_tar_gz @@ -190,7 +191,7 @@ def _get_bundle(name: str, connect_client) -> dict[str, str]: if name == "vip-plumber-test": r_versions = connect_client.r_versions() if not r_versions: - pytest.skip("No R versions available on Connect — cannot deploy Plumber") + attest.not_applicable("No R versions available on Connect — cannot deploy Plumber") manifest = json.loads((pathlib.Path(__file__).parent / "plumber_manifest.json").read_text()) manifest["platform"] = _latest_version(r_versions) return { @@ -201,7 +202,7 @@ def _get_bundle(name: str, connect_client) -> dict[str, str]: if name == "vip-quarto-test": quarto_versions = connect_client.quarto_versions() if not quarto_versions: - pytest.skip("No Quarto installations available on Connect") + attest.not_applicable("No Quarto installations available on Connect") r_versions = connect_client.r_versions() manifest: dict = { "version": 1, @@ -226,13 +227,13 @@ def _get_bundle(name: str, connect_client) -> dict[str, str]: if name == "vip-shiny-test": r_versions = connect_client.r_versions() if not r_versions: - pytest.skip("No R versions available on Connect — cannot deploy Shiny") + attest.not_applicable("No R versions available on Connect — cannot deploy Shiny") return build_shiny_bundle_files(r_versions) if name == "vip-dash-test": py_versions = connect_client.python_versions() if not py_versions: - pytest.skip("No Python versions available on Connect — cannot deploy Dash") + attest.not_applicable("No Python versions available on Connect — cannot deploy Dash") return { "app.py": ( 'import dash\napp = dash.Dash(__name__)\napp.layout = dash.html.Div("VIP test")\n' @@ -257,7 +258,7 @@ def _get_bundle(name: str, connect_client) -> dict[str, str]: if name == "vip-rmarkdown-test": r_versions = connect_client.r_versions() if not r_versions: - pytest.skip("No R versions available on Connect — cannot deploy R Markdown") + attest.not_applicable("No R versions available on Connect — cannot deploy R Markdown") # Use the pre-built manifest with the full transitive dependency closure # (rmarkdown → knitr → evaluate/highr/xfun/yaml, bslib, stringr, etc.). # An incomplete ``packages`` block causes packrat-restore to fail with @@ -278,7 +279,9 @@ def _get_bundle(name: str, connect_client) -> dict[str, str]: if name == "vip-jupyter-test": py_versions = connect_client.python_versions() if not py_versions: - pytest.skip("No Python versions available on Connect — cannot deploy Jupyter Notebook") + attest.not_applicable( + "No Python versions available on Connect — cannot deploy Jupyter Notebook" + ) notebook_content = json.dumps( { "nbformat": 4, @@ -347,7 +350,7 @@ def _get_bundle(name: str, connect_client) -> dict[str, str]: if name == "vip-fastapi-test": py_versions = connect_client.python_versions() if not py_versions: - pytest.skip("No Python versions available on Connect — cannot deploy FastAPI") + attest.not_applicable("No Python versions available on Connect — cannot deploy FastAPI") return { "app.py": ( "from fastapi import FastAPI\n" @@ -445,14 +448,16 @@ def upload_and_deploy(connect_client, deploy_state): def link_git_repository(connect_client, deploy_state): quarto_versions = connect_client.quarto_versions() if not quarto_versions: - pytest.skip("No Quarto on Connect — cannot deploy git-backed Quarto document") + attest.not_applicable("No Quarto on Connect — cannot deploy git-backed Quarto document") # Check that the remote repository is reachable before attempting to link. try: resp = httpx.head(_GIT_REPO_URL, follow_redirects=True, timeout=10) if resp.status_code >= 400: - pytest.skip(f"Git repository not reachable (HTTP {resp.status_code}): {_GIT_REPO_URL}") + attest.unproven( + f"Git repository not reachable (HTTP {resp.status_code}): {_GIT_REPO_URL}" + ) except httpx.TransportError as exc: - pytest.skip(f"Git repository not reachable: {exc}") + attest.unproven(f"Git repository not reachable: {exc}") connect_client.set_repository( deploy_state["guid"], _GIT_REPO_URL, branch=_GIT_BRANCH, directory=_GIT_DIRECTORY ) @@ -623,7 +628,7 @@ def content_renders_expected_output(connect_client, deploy_state): content = connect_client.get_content(deploy_state["guid"]) url = content.get("content_url", "") if not url: - pytest.skip("Content URL not available — skipping output verification") + attest.unproven("Content URL not available — skipping output verification") if expected["type"] == "json": # Plumber: append the route path and verify JSON response. diff --git a/src/vip_tests/cross_product/test_ssl.py b/src/vip_tests/cross_product/test_ssl.py index 1323614f..40430363 100644 --- a/src/vip_tests/cross_product/test_ssl.py +++ b/src/vip_tests/cross_product/test_ssl.py @@ -14,9 +14,10 @@ from urllib.parse import urlparse import httpx -import pytest from pytest_bdd import parsers, scenarios, then, when +from vip import attest + # --------------------------------------------------------------------------- # Scenarios # --------------------------------------------------------------------------- @@ -34,16 +35,16 @@ def check_ssl_cert(product, vip_config): product_key = product.lower().replace(" ", "_") pc = vip_config.product_config(product_key) if not pc.is_configured: - pytest.skip(f"{product} is not configured") + attest.not_applicable(f"{product} is not configured") product_url = pc.url parsed = urlparse(product_url) if parsed.scheme != "https": - pytest.skip(f"URL is not HTTPS: {product_url}") + attest.not_applicable(f"URL is not HTTPS: {product_url}") if vip_config.insecure: # The user explicitly disabled certificate verification # (tls.insecure=true) — asserting cert validity contradicts that. #268. - pytest.skip("tls.insecure=true — certificate validity checks are disabled") + attest.not_applicable("tls.insecure=true — certificate validity checks are disabled") hostname = parsed.hostname port = parsed.port or 443 @@ -55,7 +56,7 @@ def check_ssl_cert(product, vip_config): # host is unreachable, which is not a certificate finding. Same # connect-vs-handshake split as ``_attempt_tls``/``_ConnectError`` # below: only the TCP connect stage is skip-worthy. #555. - pytest.skip(f"Could not connect to {hostname}:{port}: {exc}") + attest.unproven(f"Could not connect to {hostname}:{port}: {exc}") ctx = ssl.create_default_context() try: @@ -149,12 +150,12 @@ def request_http(product, vip_config): product_key = product.lower().replace(" ", "_") pc = vip_config.product_config(product_key) if not pc.is_configured: - pytest.skip(f"{product} is not configured") + attest.not_applicable(f"{product} is not configured") product_url = pc.url if urlparse(product_url).scheme != "https": # No HTTPS endpoint to redirect to on an HTTP-only deployment. See #268. - pytest.skip(f"URL is not HTTPS: {product_url}") + attest.not_applicable(f"URL is not HTTPS: {product_url}") http_url = product_url.replace("https://", "http://", 1) parsed = urlparse(http_url) if parsed.scheme != "http": @@ -365,12 +366,12 @@ def attempt_tls_connection(product, vip_config): product_key = product.lower().replace(" ", "_") pc = vip_config.product_config(product_key) if not pc.is_configured: - pytest.skip(f"{product} is not configured") + attest.not_applicable(f"{product} is not configured") product_url = pc.url parsed = urlparse(product_url) if parsed.scheme != "https": - pytest.skip(f"URL is not HTTPS: {product_url}") + attest.not_applicable(f"URL is not HTTPS: {product_url}") hostname = parsed.hostname port = parsed.port or 443 @@ -396,7 +397,7 @@ def attempt_tls_connection(product, vip_config): ), } except _ConnectError as exc: - pytest.skip(f"Could not reach {hostname}:{port}: {exc}") + attest.unproven(f"Could not reach {hostname}:{port}: {exc}") unsupported = [ label @@ -404,7 +405,7 @@ def attempt_tls_connection(product, vip_config): if results[key]["status"] == "client_unsupported" ] if unsupported: - pytest.skip( + attest.unproven( f"Runner cannot configure {', '.join(unsupported)} — cannot " f"assess server TLS enforcement on this client." ) diff --git a/src/vip_tests/prerequisites/test_versions.py b/src/vip_tests/prerequisites/test_versions.py index d703418f..c5a4510a 100644 --- a/src/vip_tests/prerequisites/test_versions.py +++ b/src/vip_tests/prerequisites/test_versions.py @@ -2,9 +2,10 @@ from __future__ import annotations -import pytest from pytest_bdd import given, scenario, then, when +from vip import attest + @scenario("test_versions.feature", "Connect version matches configuration") def test_connect_version(): @@ -33,9 +34,9 @@ def test_package_manager_version(): ) def connect_version_configured(vip_config): if not vip_config.connect.is_configured: - pytest.skip("Connect is not configured") + attest.not_applicable("Connect is not configured") if not vip_config.connect.version: - pytest.skip( + attest.not_applicable( "No Connect version configured in vip.toml — set connect.version to enable this check" ) return vip_config.connect.version @@ -47,9 +48,9 @@ def connect_version_configured(vip_config): ) def pm_version_configured(vip_config): if not vip_config.package_manager.is_configured: - pytest.skip("Package Manager is not configured") + attest.not_applicable("Package Manager is not configured") if not vip_config.package_manager.version: - pytest.skip( + attest.not_applicable( "No Package Manager version configured in vip.toml — " "set package_manager.version to enable this check" ) @@ -61,7 +62,7 @@ def fetch_connect_version(connect_client): info = connect_client.server_settings() version = info.get("version") if not version: - pytest.skip("Connect server_settings did not return a version field") + attest.unproven("Connect server_settings did not return a version field") return version @@ -70,7 +71,7 @@ def fetch_pm_version(pm_client): info = pm_client.status() version = info.get("version") if not version: - pytest.skip("Package Manager status endpoint did not return a version field") + attest.unproven("Package Manager status endpoint did not return a version field") return version diff --git a/src/vip_tests/workbench/test_ide_launch.py b/src/vip_tests/workbench/test_ide_launch.py index aa41c779..b0870370 100644 --- a/src/vip_tests/workbench/test_ide_launch.py +++ b/src/vip_tests/workbench/test_ide_launch.py @@ -15,6 +15,7 @@ from playwright.sync_api import TimeoutError as PlaywrightTimeoutError from pytest_bdd import given, scenario, then, when +from vip import attest from vip_tests.workbench.conftest import ( TIMEOUT_CLEANUP, TIMEOUT_CODE_EXEC, @@ -253,7 +254,7 @@ def _dismiss_dialog_and_skip(page: Page, reason: str) -> NoReturn: pass except (PlaywrightTimeoutError, PlaywrightError): pass - pytest.skip(reason) + attest.not_applicable(reason) @then("the session transitions to Active state") @@ -321,7 +322,7 @@ def _expect_ide_or_skip( ) else: reason = skip_reason.format(exc=exc) if "{exc}" in skip_reason else skip_reason - pytest.skip(reason) + attest.not_applicable(reason) @then("the VS Code IDE is displayed") @@ -538,7 +539,7 @@ def jupyterlab_executes_code(page: Page, session_context: dict): pass # fall through; re-check below and retry or skip notebook_card = page.locator(JupyterLabSession.LAUNCHER_NOTEBOOK_CARD).first if notebook_card.count() == 0: - pytest.skip("No notebook kernel cards available in JupyterLab launcher") + attest.unproven("No notebook kernel cards available in JupyterLab launcher") expect(notebook_card).to_be_visible(timeout=TIMEOUT_CODE_EXEC) # A leftover modal from a *previous* notebook in this session (e.g. an @@ -603,7 +604,7 @@ def jupyterlab_executes_code(page: Page, session_context: dict): # Type-and-run with input verification and run retries. Returns False only # after exhausting retries with no output. if not _run_jupyter_cell_and_get_output(page, notebook_panel, cell_input): - pytest.skip( + attest.unproven( "JupyterLab notebook UI did not surface cell output within timeout — " "the cell run raced session hydration (kernel reachability is verified " "separately; this is a UI-interaction timeout, not kernel death)" @@ -627,7 +628,7 @@ def positron_console_accessible(page: Page): reason rather than implying Positron is missing. """ if not ensure_positron_console(page, timeout=TIMEOUT_IDE_LOAD): - pytest.skip( + attest.unproven( "Positron loaded but no console session could be started — the " '"Start New Console Session" control was unavailable, no R/Python ' "interpreter resolved, or the console did not render (issue #477)." diff --git a/src/vip_tests/workbench/test_jobs.py b/src/vip_tests/workbench/test_jobs.py index 119619b1..0977c56d 100644 --- a/src/vip_tests/workbench/test_jobs.py +++ b/src/vip_tests/workbench/test_jobs.py @@ -16,6 +16,7 @@ from playwright.sync_api import TimeoutError as PlaywrightTimeoutError from pytest_bdd import given, scenario, then, when +from vip import attest from vip.config import VIPConfig from vip_tests.workbench.conftest import ( TIMEOUT_CLEANUP, @@ -143,7 +144,7 @@ def start_rstudio_session_for_job(page: Page, job_context: dict): cancel.click(timeout=TIMEOUT_QUICK) except Exception: pass - pytest.skip("RStudio Pro IDE not available in this Workbench deployment") + attest.not_applicable("RStudio Pro IDE not available in this Workbench deployment") ide_tab.click(timeout=TIMEOUT_QUICK) @@ -157,7 +158,7 @@ def start_rstudio_session_for_job(page: Page, job_context: dict): cancel.click(timeout=TIMEOUT_QUICK) except Exception: pass - pytest.skip( + attest.unproven( "RStudio Pro tab opened but Launch button did not appear — " "the IDE may not be installed or fully available on this Workbench instance" ) @@ -283,7 +284,7 @@ def run_as_background_job(page: Page, job_context: dict): try: bg_tab.wait_for(state="visible", timeout=TIMEOUT_DIALOG) except PlaywrightTimeoutError: - pytest.skip( + attest.not_applicable( "Background Jobs tab not found — Background Jobs may not be available " "in this Workbench configuration" ) @@ -294,7 +295,7 @@ def run_as_background_job(page: Page, job_context: dict): try: start_btn.wait_for(state="visible", timeout=TIMEOUT_DIALOG) except PlaywrightTimeoutError: - pytest.skip("Start Background Job button not found — cannot submit background job") + attest.unproven("Start Background Job button not found — cannot submit background job") start_btn.click() # Fill in the script path. @@ -302,7 +303,7 @@ def run_as_background_job(page: Page, job_context: dict): try: script_input.wait_for(state="visible", timeout=TIMEOUT_DIALOG) except PlaywrightTimeoutError: - pytest.skip("Background Job script input not found") + attest.unproven("Background Job script input not found") script_input.fill(_JOB_SCRIPT_PATH) # Submit the job. @@ -321,7 +322,7 @@ def run_as_workbench_job(page: Page, job_context: dict): try: wb_tab.wait_for(state="visible", timeout=TIMEOUT_DIALOG) except PlaywrightTimeoutError: - pytest.skip( + attest.not_applicable( "Workbench Jobs tab not found — Workbench Jobs (Launcher) may not be available " "in this Workbench configuration" ) @@ -332,7 +333,7 @@ def run_as_workbench_job(page: Page, job_context: dict): try: new_btn.wait_for(state="visible", timeout=TIMEOUT_DIALOG) except PlaywrightTimeoutError: - pytest.skip("Run Script as Workbench Job button not found") + attest.unproven("Run Script as Workbench Job button not found") _open_workbench_job_dialog(page, new_btn) # Select the script via the file chooser. Unlike the Background Job dialog, @@ -408,7 +409,7 @@ def _select_workbench_job_script(page: Page, script_filename: str) -> None: try: browse_btn.wait_for(state="visible", timeout=TIMEOUT_DIALOG) except PlaywrightTimeoutError: - pytest.skip("Workbench Job script Browse button not found in the submission dialog") + attest.unproven("Workbench Job script Browse button not found in the submission dialog") browse_btn.click() # The Choose File dialog's name field IS editable -- type the filename there. @@ -416,7 +417,7 @@ def _select_workbench_job_script(page: Page, script_filename: str) -> None: try: name_input.wait_for(state="visible", timeout=TIMEOUT_DIALOG) except PlaywrightTimeoutError: - pytest.skip("Workbench Job file chooser did not open") + attest.unproven("Workbench Job file chooser did not open") # The chooser clears the name field ~1s after it first appears, as GWT # finishes initializing it. A fill() that lands before that reset is wiped, diff --git a/src/vip_tests/workbench/test_session_capacity.py b/src/vip_tests/workbench/test_session_capacity.py index ed4b0650..7302601d 100644 --- a/src/vip_tests/workbench/test_session_capacity.py +++ b/src/vip_tests/workbench/test_session_capacity.py @@ -22,6 +22,7 @@ from playwright.sync_api import Page, expect from pytest_bdd import scenarios, then, when +from vip import attest from vip_tests.workbench.conftest import ( TIMEOUT_DIALOG, TIMEOUT_QUICK, @@ -143,7 +144,7 @@ def _launch_session( raise ResourceProfileDisabled(profile) option.click(timeout=TIMEOUT_QUICK) else: - pytest.skip(f"Resource profile dropdown not available; cannot select '{profile}'") + attest.unproven(f"Resource profile dropdown not available; cannot select '{profile}'") # Fill session name. page.fill(NewSessionDialog.SESSION_NAME, session_name) @@ -181,7 +182,7 @@ def launch_sessions(page: Page, vip_config): # Every profile is offered but disabled for this user — nothing # is launchable, so there is no capacity to exercise. names = ", ".join(p.name for p in detected) - pytest.skip( + attest.not_applicable( f"All resource profiles are disabled for the authenticated user: {names}" ) profiles_to_test = enabled @@ -220,7 +221,7 @@ def launch_sessions(page: Page, vip_config): # skipped (not passed) on a correctly-restricted test account, # distinct from an actual capacity failure. names = ", ".join(disabled_profiles) - pytest.skip( + attest.not_applicable( f"Resource profile(s) '{names}' are disabled for the authenticated " "user (likely a group/entitlement restriction)" ) diff --git a/src/vip_tests/workbench/test_session_capacity_k8s.py b/src/vip_tests/workbench/test_session_capacity_k8s.py index 50fb2e23..ec5c6d2d 100644 --- a/src/vip_tests/workbench/test_session_capacity_k8s.py +++ b/src/vip_tests/workbench/test_session_capacity_k8s.py @@ -22,6 +22,7 @@ from playwright.sync_api import Page, expect from pytest_bdd import given, scenarios, then, when +from vip import attest from vip.clients.kubernetes import KubernetesClient from vip_tests.workbench.conftest import ( TIMEOUT_DIALOG, @@ -84,7 +85,7 @@ def _launch_session(page: Page, session_name: str, profile: str | None = None) - raise ResourceProfileDisabled(profile) option.click(timeout=TIMEOUT_QUICK) else: - pytest.skip(f"Resource profile dropdown not available; cannot select '{profile}'") + attest.unproven(f"Resource profile dropdown not available; cannot select '{profile}'") page.fill(NewSessionDialog.SESSION_NAME, session_name) @@ -135,30 +136,34 @@ def _parse_memory_gib(mem_str: str) -> float: def k8s_cluster_configured(vip_config) -> KubernetesClient: k8s_cfg = vip_config.workbench.kubernetes if not k8s_cfg.is_configured: - pytest.skip("workbench.kubernetes is not configured (set enabled = true in vip.toml)") + attest.not_applicable( + "workbench.kubernetes is not configured (set enabled = true in vip.toml)" + ) try: return KubernetesClient(namespace=k8s_cfg.namespace) except RuntimeError as exc: - pytest.skip(str(exc)) + attest.unproven(str(exc)) @given("a maximum session count is configured") def max_session_count_configured(vip_config): if vip_config.workbench.kubernetes.max_sessions is None: - pytest.skip("workbench.kubernetes.max_sessions is not set in vip.toml") + attest.not_applicable("workbench.kubernetes.max_sessions is not set in vip.toml") @given("node-pool-to-profile mappings are configured") def node_pool_profiles_configured(vip_config): if not vip_config.workbench.kubernetes.node_pool_profiles: - pytest.skip("workbench.kubernetes.node_pool_profiles is not configured in vip.toml") + attest.not_applicable( + "workbench.kubernetes.node_pool_profiles is not configured in vip.toml" + ) @given("resource limit expectations are configured") def resource_limits_configured(vip_config): k8s_cfg = vip_config.workbench.kubernetes if not k8s_cfg.profile_cpu_limit and not k8s_cfg.profile_memory_limit_gib: - pytest.skip( + attest.not_applicable( "workbench.kubernetes.profile_cpu_limit / profile_memory_limit_gib " "are not configured in vip.toml" ) @@ -242,7 +247,7 @@ def launch_profiled_session(page: Page, vip_config) -> list[dict]: try: _launch_session(page, name, profile=profile) except ResourceProfileDisabled as exc: - pytest.skip( + attest.not_applicable( f"Resource profile '{exc.profile}' is disabled for the " "authenticated user (likely a group/entitlement restriction)" ) @@ -263,7 +268,7 @@ def launch_limited_session(page: Page, vip_config) -> list[dict]: try: _launch_session(page, name, profile=profile) except ResourceProfileDisabled as exc: - pytest.skip( + attest.not_applicable( f"Resource profile '{exc.profile}' is disabled for the " "authenticated user (likely a group/entitlement restriction)" ) diff --git a/src/vip_tests/workbench/test_sessions.py b/src/vip_tests/workbench/test_sessions.py index 223aaf59..b6bbb564 100644 --- a/src/vip_tests/workbench/test_sessions.py +++ b/src/vip_tests/workbench/test_sessions.py @@ -13,6 +13,7 @@ from playwright.sync_api import Page, expect from pytest_bdd import given, scenario, then, when +from vip import attest from vip_tests.workbench.conftest import ( TIMEOUT_CLEANUP, TIMEOUT_DIALOG, @@ -221,7 +222,7 @@ def session_becomes_active_again(page: Page, workbench_url: str, session_context page.reload(timeout=TIMEOUT_PAGE_LOAD) expect(page.locator(Homepage.POSIT_LOGO)).to_be_visible(timeout=TIMEOUT_PAGE_LOAD) - pytest.skip( + attest.unproven( f"Session did not return to Active state after resume — " f"suspend/resume may not be supported in this Workbench configuration ({exc})" ) From 4542bdcf38f5d04c3e1648c482b1286abae97647 Mon Sep 17 00:00:00 2001 From: Ian Flores Siaca <18703558+ian-flores@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:52:11 -0700 Subject: [PATCH 2/2] test: harden the skip-triage guard and fix two reasons it flagged Copilot review on #623, all four findings real. The guard scanned line by line for pytest.skip(, so the same call split across two lines -- or written with whitespace around the dot -- walked straight past it. A guard that a reformat can evade is not a guard. Scan the whole file with a whitespace-tolerant pattern instead, and extract find_bare_skips/imports_attest as plain functions so the guard's own behaviour is testable rather than asserted only against the real tree. The import check asserted the substring "attest" appeared somewhere in the file, which a comment or docstring satisfies. It now requires an actual import statement. Two source fixes. A helper docstring still promised pytest.skip after the call became attest.not_applicable. And the RStudio Pro job skip still read "the IDE may not be installed" on a branch only reachable after the tab opened and proved it is -- that contradiction is precisely why the site was reclassified as unproven, so leaving the wording was a half-fix. --- selftests/test_skip_triage.py | 75 ++++++++++++++++++++-- src/vip_tests/workbench/test_ide_launch.py | 2 +- src/vip_tests/workbench/test_jobs.py | 6 +- 3 files changed, 73 insertions(+), 10 deletions(-) diff --git a/selftests/test_skip_triage.py b/selftests/test_skip_triage.py index 3374c792..576f9483 100644 --- a/selftests/test_skip_triage.py +++ b/selftests/test_skip_triage.py @@ -32,18 +32,37 @@ "workbench/test_sessions.py", ] -_BARE_SKIP = re.compile(r"\bpytest\.skip\s*\(") +# Tolerates whitespace anywhere the parser does, including a newline, so the +# guard cannot be evaded by reformatting. Scanned over the whole file rather +# than line by line for the same reason. ``\b`` before ``pytest`` keeps +# ``request.node.skip(...)`` and similar attribute calls out of it. +_BARE_SKIP = re.compile(r"\bpytest\s*\.\s*skip\s*\(") +_IMPORTS_ATTEST = re.compile( + r"^\s*(?:from\s+vip\s+import\s+.*\battest\b|import\s+vip\.attest\b)", re.M +) + + +def find_bare_skips(text: str) -> list[int]: + """Line numbers of every bare ``pytest.skip(`` call in *text*. + + A ``pytest.skip`` written inside a comment or docstring counts too. That + is a deliberate false positive: it is loud and trivially reworded, whereas + missing a real one is silent, and silence is the failure mode this whole + guard exists to prevent. + """ + return [text[: m.start()].count("\n") + 1 for m in _BARE_SKIP.finditer(text)] + + +def imports_attest(text: str) -> bool: + """True when *text* really imports the helpers, not merely mentions them.""" + return _IMPORTS_ATTEST.search(text) is not None @pytest.mark.parametrize("relpath", TRIAGED_FILES) def test_triaged_file_has_no_unclassified_skip(relpath: str): path = _SRC / relpath assert path.exists(), f"{relpath} moved or was deleted; update TRIAGED_FILES" - offenders = [ - f"{relpath}:{n}" - for n, line in enumerate(path.read_text().splitlines(), 1) - if _BARE_SKIP.search(line) - ] + offenders = [f"{relpath}:{n}" for n in find_bare_skips(path.read_text())] assert not offenders, ( "bare pytest.skip() in a triaged file -- say which kind of skip this is " "with attest.unproven() or attest.not_applicable():\n " + "\n ".join(offenders) @@ -54,7 +73,49 @@ def test_triaged_file_has_no_unclassified_skip(relpath: str): def test_triaged_file_actually_uses_the_helpers(relpath: str): """Guards against 'triaging' a file by deleting its skips.""" text = (_SRC / relpath).read_text() - assert "attest" in text, f"{relpath} is listed as triaged but never imports attest" + assert imports_attest(text), f"{relpath} is listed as triaged but never imports attest" + + +class TestBareSkipDetection: + """The guard's own tests. A guard that is easy to evade is not a guard.""" + + def test_finds_a_plain_call(self): + assert find_bare_skips("x = 1\npytest.skip('no')\n") == [2] + + def test_finds_a_call_split_across_lines(self): + # ruff would not format it this way, but a hand edit can, and a + # line-by-line scan misses it entirely. + assert find_bare_skips("pytest.\nskip('no')\n") == [1] + + def test_finds_a_call_with_whitespace_around_the_dot(self): + assert find_bare_skips("pytest . skip ('no')\n") == [1] + + def test_ignores_the_attest_helpers(self): + text = "attest.unproven('a')\nattest.not_applicable('b')\n" + assert find_bare_skips(text) == [] + + def test_ignores_an_unrelated_skip_attribute(self): + assert find_bare_skips("request.node.skip('x')\nself.skip()\n") == [] + + def test_reports_every_occurrence_in_order(self): + text = "pytest.skip('a')\nx = 2\npytest.skip('b')\n" + assert find_bare_skips(text) == [1, 3] + + +class TestAttestImportDetection: + """ "attest" appearing anywhere is not evidence the file uses the helpers.""" + + def test_accepts_the_real_import(self): + assert imports_attest("from vip import attest\n") + + def test_accepts_a_module_import(self): + assert imports_attest("import vip.attest\n") + + def test_rejects_a_mere_mention_in_a_comment(self): + assert not imports_attest("# remember to use attest here\nimport pytest\n") + + def test_rejects_a_mention_in_a_docstring(self): + assert not imports_attest('"""Uses attest for skips."""\nimport pytest\n') def test_triage_list_has_no_duplicates(): diff --git a/src/vip_tests/workbench/test_ide_launch.py b/src/vip_tests/workbench/test_ide_launch.py index b0870370..ff2786e2 100644 --- a/src/vip_tests/workbench/test_ide_launch.py +++ b/src/vip_tests/workbench/test_ide_launch.py @@ -235,7 +235,7 @@ def _start_session(page: Page, ide_type: str, session_name: str): def _dismiss_dialog_and_skip(page: Page, reason: str) -> NoReturn: - """Best-effort cancel of the New Session dialog, then ``pytest.skip``. + """Best-effort cancel of the New Session dialog, then ``attest.not_applicable``. Uses a short timeout on the cancel click so a missing or unreachable cancel button does not mask the real skip reason with a 30-second diff --git a/src/vip_tests/workbench/test_jobs.py b/src/vip_tests/workbench/test_jobs.py index 0977c56d..41418049 100644 --- a/src/vip_tests/workbench/test_jobs.py +++ b/src/vip_tests/workbench/test_jobs.py @@ -159,8 +159,10 @@ def start_rstudio_session_for_job(page: Page, job_context: dict): except Exception: pass attest.unproven( - "RStudio Pro tab opened but Launch button did not appear — " - "the IDE may not be installed or fully available on this Workbench instance" + "RStudio Pro tab opened but its Launch button never appeared, so no " + "session could be started and the Background Jobs checks below never " + "ran. The tab opening means the IDE is present; this is a UI or " + "readiness problem, not a missing IDE." ) page.fill(NewSessionDialog.SESSION_NAME, session_name)