diff --git a/.github/instructions/test.instructions.md b/.github/instructions/test.instructions.md index e83b7bb452..26bd1650a4 100644 --- a/.github/instructions/test.instructions.md +++ b/.github/instructions/test.instructions.md @@ -35,6 +35,14 @@ Most tests should be unit tests. Integration and end-to-end tests are for testin - No network calls should ever happen in unit tests, but file access is okay - Unit tests should be fast and can be run in parallel. +## Correctness Test Timeouts + +- For unit-test subprocesses whose timeout only prevents a hang, use `TEST_WATCHDOG_SECONDS` from `unit.async_utils` rather than a short wall-clock deadline. The shared watchdog allows for startup, filesystem, and scheduling delays on busy CI workers; it is not an expected runtime. +- Use the same watchdog for thread-barrier rendezvous that test concurrency correctness, not worker startup speed. Keep the barrier and assertions that require distinct workers. +- Preserve intentional performance limits, operation deadlines, timeout/cancellation assertions, and JavaScript VM execution limits. Do not increase them merely to make a test pass. +- For a single `node:test` suite that does not need child-process isolation, run `node --test-reporter=tap ` without `--test` to avoid an extra Node process. Preserve fresh subprocesses where isolation is part of the test contract, such as import guards. +- Still require a successful process exit and, for Node suites, a positive collected-test count with every collected test passing. A longer watchdog must not hide failures, skipped tests, or an empty suite. + ## Integration Test Rules - Mark with `@pytest.mark.run_only_if_all_tests` (skipped unless `RUN_ALL_TESTS=true`) diff --git a/doc/contributing/4_running_tests.md b/doc/contributing/4_running_tests.md index 2606b9eada..aa4b11ddc0 100644 --- a/doc/contributing/4_running_tests.md +++ b/doc/contributing/4_running_tests.md @@ -58,14 +58,30 @@ cancellation when a matrix job has already failed. The CodeQL job needs Fork PRs use native CodeQL publishing and GitHub's read-only fork token policy. Run the deterministic, offline regression coverage with Node.js installed. -The pytest entry point runs the Node scenario tests and checks workflow wiring: +The pytest entry point runs the Node scenario tests and checks workflow wiring. +It runs `node:test` in a single Node process, with the shared 300-second correctness +watchdog to allow for startup and filesystem delays on busy Windows runners. +The watchdog is a hang backstop, not a performance requirement; API calls and +polling delays remain mocked. ```bash uv run --frozen pytest tests/unit/build_scripts/test_codeql_merge_group.py -q ``` The helper's scenarios can also run directly with -`node --test tests/unit/build_scripts/test_codeql_merge_group.cjs`. +`node --test-reporter=tap tests/unit/build_scripts/test_codeql_merge_group.cjs`. + +The documentation JavaScript suites in `test_docs_javascript.py` also use one +Node process per suite and the shared correctness watchdog. Their per-VM +execution limits remain unchanged. + +The Bash deployment helper in `test_code_deployment.py` uses the same shared +watchdog for subprocess completion. Readiness and rollback tests retain their +simulated operation deadlines, polling assertions, and signal exit-code checks. + +The seeded-converter worker-thread test also uses the shared watchdog for its +two-worker barrier. It still requires distinct random generators and identical +seeded draws; worker startup speed is not part of that contract. ## Running a subset while iterating @@ -84,6 +100,10 @@ release any test-controlled waits and join their tasks before removing mocks or disposing fixtures, including when a watchdog expires. When waiting for a task to signal readiness, also watch for that task finishing so an early exception is not hidden by the readiness timeout. +The analytics quick-lane capacity test observes request admission and completion +with these shared watchdogs, then releases its reader gate and drains all requests +before restoring mocks. Its 503/Retry-After and independent-execution assertions +remain unchanged. Use `wait_for_event_async` to observe readiness and `wait_for_completion_async` to observe the outcome, including failures after readiness. Observation timeouts do not cancel owned work. Release gates before calling `settle_tasks_async` in `finally`. diff --git a/tests/unit/backend/test_analytics_routes.py b/tests/unit/backend/test_analytics_routes.py index 033ede82e4..8e1b6c9e9e 100644 --- a/tests/unit/backend/test_analytics_routes.py +++ b/tests/unit/backend/test_analytics_routes.py @@ -43,7 +43,7 @@ AttackAnalyticsResultsQuery, AttackOutcome, ) -from unit.async_utils import TEST_WATCHDOG_SECONDS, wait_for_completion_async, wait_for_event_async +from unit.async_utils import TEST_WATCHDOG_SECONDS, settle_tasks_async, wait_for_completion_async, wait_for_event_async from unit.memory.test_attack_analytics import make_result @@ -447,18 +447,18 @@ async def blocked_async(**kwargs: object) -> Any: asyncio.create_task(analytics_client.post("/api/analytics/attacks/results", json={})) for _ in range(12) ] try: - await asyncio.wait_for(admitted.wait(), 10) + await wait_for_event_async(event=admitted, tasks=requests) rejected = await analytics_client.post("/api/analytics/attacks/results", json={}) assert rejected.status_code == 503 assert rejected.headers["Retry-After"] == "1" assert analytics_service.has_active_work release.set() - responses = await asyncio.wait_for(asyncio.gather(*requests), 30) + responses = await wait_for_completion_async(future=asyncio.gather(*requests)) assert all(response.status_code == 200 for response in responses) assert read.await_count == 12 finally: release.set() - await asyncio.gather(*requests, return_exceptions=True) + await settle_tasks_async(*requests) @pytest.mark.parametrize( diff --git a/tests/unit/build_scripts/test_codeql_merge_group.py b/tests/unit/build_scripts/test_codeql_merge_group.py index f6be8d6054..45f203f7b4 100644 --- a/tests/unit/build_scripts/test_codeql_merge_group.py +++ b/tests/unit/build_scripts/test_codeql_merge_group.py @@ -4,6 +4,7 @@ from __future__ import annotations +import re import subprocess from pathlib import Path from typing import Any @@ -11,6 +12,7 @@ import pytest import yaml +from unit.async_utils import TEST_WATCHDOG_SECONDS REPO_ROOT = Path(__file__).resolve().parents[3] CODEQL_PIN = "24c54180a607b1449ed407dd24f251e4e9147c8d" @@ -42,15 +44,19 @@ def steps() -> dict[str, Any]: def test_codeql_merge_group_scenarios(steps: dict[str, Any]) -> None: script = steps["Publish merge-group CodeQL results"]["with"]["script"] with patch.dict("os.environ", {"CODEQL_TEST_SCRIPT": script}): + # Run node:test in-process, without --test's extra child process on busy Windows runners. result = subprocess.run( - ["node", "--test", "--test-reporter=tap", str(Path(__file__).with_suffix(".cjs"))], + ["node", "--test-reporter=tap", str(Path(__file__).with_suffix(".cjs"))], cwd=REPO_ROOT, capture_output=True, text=True, check=False, - timeout=60, + timeout=TEST_WATCHDOG_SECONDS, ) assert result.returncode == 0, result.stdout + result.stderr + summary = dict(re.findall(r"^# (tests|pass) (\d+)$", result.stdout, re.MULTILINE)) + assert int(summary.get("tests", "0")) > 0, result.stdout + assert summary.get("pass") == summary["tests"], result.stdout assert "# fail 0" in result.stdout and "# skipped 0" in result.stdout diff --git a/tests/unit/build_scripts/test_docs_javascript.py b/tests/unit/build_scripts/test_docs_javascript.py index 01ce6a3445..ed9ed1c96e 100644 --- a/tests/unit/build_scripts/test_docs_javascript.py +++ b/tests/unit/build_scripts/test_docs_javascript.py @@ -2,8 +2,8 @@ # Licensed under the MIT license. """Run documentation JavaScript tests with one Node startup per suite. -The Node tests use fresh, time-limited VMs for behavior checks. The process -deadline allows for slow CI startup, not slow JavaScript execution. +The Node tests use fresh, time-limited VMs for behavior checks. The shared +correctness watchdog allows for slow CI startup, not slow JavaScript execution. Local environments without Node skip these tests. """ @@ -13,6 +13,7 @@ from pathlib import Path import pytest +from unit.async_utils import TEST_WATCHDOG_SECONDS @pytest.mark.skipif(shutil.which("node") is None, reason="node not installed") @@ -22,7 +23,7 @@ def test_docs_javascript(suite: str) -> None: ["node", "--test-reporter=tap", str(Path(__file__).with_name(suite))], capture_output=True, text=True, - timeout=60, + timeout=TEST_WATCHDOG_SECONDS, ) assert result.returncode == 0, result.stdout + result.stderr summary = dict(re.findall(r"^# (tests|pass) (\d+)$", result.stdout, re.MULTILINE)) diff --git a/tests/unit/converter/test_seeded_converter_determinism.py b/tests/unit/converter/test_seeded_converter_determinism.py index 80952b3e9a..e07309ced0 100644 --- a/tests/unit/converter/test_seeded_converter_determinism.py +++ b/tests/unit/converter/test_seeded_converter_determinism.py @@ -7,6 +7,7 @@ from collections.abc import Callable import pytest +from unit.async_utils import TEST_WATCHDOG_SECONDS from pyrit.common.random_context import configure_random_seed from pyrit.converter import ( @@ -134,7 +135,7 @@ async def convert_async(self, *, prompt: str, input_type: PromptDataType = "text barrier = threading.Barrier(2) def draw() -> tuple[random.Random, float]: - barrier.wait(timeout=5) + barrier.wait(timeout=TEST_WATCHDOG_SECONDS) rng = self._get_random_generator(stream="values") return (rng, rng.random()) diff --git a/tests/unit/infra/test_code_deployment.py b/tests/unit/infra/test_code_deployment.py index 8c7ba492c2..ba1276ccf4 100644 --- a/tests/unit/infra/test_code_deployment.py +++ b/tests/unit/infra/test_code_deployment.py @@ -12,6 +12,8 @@ import unittest from pathlib import Path +from unit.async_utils import TEST_WATCHDOG_SECONDS + REPO_ROOT = Path(__file__).resolve().parents[3] PIPELINES = REPO_ROOT / "infra" / "pipelines" SUBSCRIPTION = "11111111-1111-1111-1111-111111111111" @@ -246,7 +248,7 @@ def _run( text=True, check=False, env=environment, - timeout=30, + timeout=TEST_WATCHDOG_SECONDS, ) assert "Unexpected " not in result.stderr, result.stderr return result