Skip to content

Commit 46aaa31

Browse files
GWealecopybara-github
authored andcommitted
test: make unit contracts platform neutral
Pre-emptive: every CI job is ubuntu-latest, so none of these tests fail today. No assertion is weakened - each replacement is equivalent or stricter on Linux. Co-authored-by: George Weale <gweale@google.com> PiperOrigin-RevId: 954835278
1 parent 66cf08d commit 46aaa31

5 files changed

Lines changed: 143 additions & 49 deletions

File tree

tests/unittests/artifacts/test_artifact_service.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@
2626
from typing import Union
2727
from unittest import mock
2828
from unittest.mock import patch
29-
from urllib.parse import unquote
3029
from urllib.parse import urlparse
30+
from urllib.request import url2pathname
3131

3232
from google.adk.artifacts import file_artifact_service
3333
from google.adk.artifacts.base_artifact_service import ArtifactVersion
@@ -892,7 +892,7 @@ async def test_file_metadata_camelcase(tmp_path, artifact_service_factory):
892892
"customMetadata": {},
893893
}
894894
parsed_canonical = urlparse(metadata["canonicalUri"])
895-
canonical_path = Path(unquote(parsed_canonical.path))
895+
canonical_path = Path(url2pathname(parsed_canonical.path))
896896
assert canonical_path.name == "report.txt"
897897
assert canonical_path.read_bytes() == b"binary-content"
898898

@@ -942,7 +942,7 @@ async def test_file_list_artifact_versions(tmp_path, artifact_service_factory):
942942
assert version_meta.canonical_uri == version_payload_path.as_uri()
943943
assert version_meta.custom_metadata == custom_metadata
944944
parsed_version_uri = urlparse(version_meta.canonical_uri)
945-
version_uri_path = Path(unquote(parsed_version_uri.path))
945+
version_uri_path = Path(url2pathname(parsed_version_uri.path))
946946
assert version_uri_path.read_bytes() == b"binary-content"
947947

948948
fetched = await artifact_service.get_artifact_version(

tests/unittests/cli/test_adk_web_server_tests.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,8 @@ def test_rebuild_single_test(test_client):
127127
assert response.json() == {"status": "success"}
128128
mock_to_thread.assert_called_once()
129129
args, kwargs = mock_to_thread.call_args
130-
assert args[1].endswith("tests/my_test.json")
130+
test_dir, test_name = os.path.split(args[1])
131+
assert (os.path.basename(test_dir), test_name) == ("tests", "my_test.json")
131132

132133

133134
def test_run_tests(test_client):

tests/unittests/code_executors/test_code_execution_utils.py

Lines changed: 65 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,53 @@
1212
# See the License for the specific language governing permissions and
1313
# limitations under the License.
1414

15-
import signal
15+
import multiprocessing
16+
import time
17+
import traceback
1618

1719
from google.adk.code_executors import code_execution_utils
1820
from google.genai import types
1921

22+
# The extraction itself must finish promptly. The join budget is far looser
23+
# because it also covers spawning the child and importing this module there.
24+
_REDOS_DEADLINE_SECONDS = 2.0
25+
_CHILD_JOIN_TIMEOUT_SECONDS = 120.0
26+
27+
28+
def _exercise_redos_candidate(result_conn) -> None:
29+
"""Runs the ReDoS regression payload in an independently stoppable process."""
30+
failure = None
31+
try:
32+
ticks = "`" * 3
33+
long_invalid_payload = (
34+
ticks + "python\n" + "x = 1\n" * 5000 + "not_matching"
35+
)
36+
content = types.Content(
37+
role="model",
38+
parts=[types.Part(text=long_invalid_payload)],
39+
)
40+
delimiters = [(ticks + "python\n", "\n" + ticks)]
41+
42+
started = time.perf_counter()
43+
code = code_execution_utils.CodeExecutionUtils.extract_code_and_truncate_content(
44+
content, delimiters
45+
)
46+
elapsed = time.perf_counter() - started
47+
48+
if code is not None:
49+
failure = f"expected no code to be extracted, got {code!r}"
50+
elif elapsed > _REDOS_DEADLINE_SECONDS:
51+
failure = (
52+
f"extraction took {elapsed:.3f}s, over the"
53+
f" {_REDOS_DEADLINE_SECONDS}s deadline (possible ReDoS regression)"
54+
)
55+
except BaseException: # pylint: disable=broad-except
56+
# Without this the parent only sees a bare exit code and has to dig the
57+
# traceback out of the child's captured stderr.
58+
failure = f"extraction raised in the child:\n{traceback.format_exc()}"
59+
result_conn.send(failure)
60+
result_conn.close()
61+
2062

2163
def test_extract_code_and_truncate_content_basic():
2264
"""Tests basic code extraction and content truncation."""
@@ -94,29 +136,30 @@ def test_extract_code_and_truncate_content_no_delimiter():
94136

95137
def test_extract_code_and_truncate_content_redos_vulnerability():
96138
"""Tests that a string that would cause ReDoS behaves reasonably."""
97-
# Construct a long string that contains repeating patterns without matching delimiters.
98-
# The old regex pattern would backtrack exponentially.
99-
ticks = "`" * 3
100-
long_invalid_payload = ticks + "python\n" + "x = 1\n" * 5000 + "not_matching"
101-
content = types.Content(
102-
role="model",
103-
parts=[types.Part(text=long_invalid_payload)],
104-
)
105-
delimiters = [(ticks + "python\n", "\n" + ticks)]
106-
107-
def handler(_signum, _frame):
108-
raise TimeoutError("Test timed out (possible ReDoS regression)")
109-
110-
signal.signal(signal.SIGALRM, handler)
111-
signal.alarm(2)
139+
context = multiprocessing.get_context("spawn")
140+
receiver, sender = context.Pipe(duplex=False)
141+
process = context.Process(target=_exercise_redos_candidate, args=(sender,))
142+
process.start()
143+
sender.close()
144+
process.join(timeout=_CHILD_JOIN_TIMEOUT_SECONDS)
145+
hung = process.is_alive()
146+
if hung:
147+
process.kill()
148+
process.join()
149+
exitcode = process.exitcode
112150
try:
113-
# If ReDoS vulnerability exists, this call will hang or take a very long time.
114-
code = code_execution_utils.CodeExecutionUtils.extract_code_and_truncate_content(
115-
content, delimiters
116-
)
151+
# poll() is also true at EOF, so recv() has to carry the "child died
152+
# without reporting" case rather than a poll() guard.
153+
failure = receiver.recv()
154+
except EOFError:
155+
failure = "child exited without reporting a result"
117156
finally:
118-
signal.alarm(0)
119-
assert code is None
157+
receiver.close()
158+
process.close()
159+
160+
assert not hung, "extraction never returned (possible ReDoS regression)"
161+
assert failure is None, failure
162+
assert exitcode == 0, f"extraction process exited with {exitcode}"
120163

121164

122165
def test_extract_code_and_truncate_content_multiple_delimiter_pairs():

tests/unittests/sessions/migration/test_migration.py

Lines changed: 59 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
from __future__ import annotations
1717

18+
import contextlib
1819
from datetime import datetime
1920
from datetime import timezone
2021
import os
@@ -368,24 +369,69 @@ def test_migrate_from_sqlalchemy_pickle_ignores_non_object_json_fields():
368369
assert event.content is None
369370

370371

371-
def test_migrate_from_sqlalchemy_pickle_reads_naive_timestamp_as_local(
372-
monkeypatch,
373-
):
372+
@contextlib.contextmanager
373+
def _pinned_local_timezone(name: str):
374+
"""Pins the process timezone for the duration of the block.
375+
376+
``time.tzset`` is POSIX-only, so on other platforms the block runs in the
377+
host zone instead. Restoring ``TZ`` without a second ``tzset`` would leave
378+
the C library pinned for the rest of the session, so both are undone.
379+
"""
380+
if not hasattr(time, "tzset"):
381+
yield
382+
return
383+
previous = os.environ.get("TZ")
384+
os.environ["TZ"] = name
385+
time.tzset()
386+
try:
387+
yield
388+
finally:
389+
if previous is None:
390+
os.environ.pop("TZ", None)
391+
else:
392+
os.environ["TZ"] = previous
393+
time.tzset()
394+
395+
396+
def test_migrate_from_sqlalchemy_pickle_reads_naive_timestamp_as_local():
374397
"""Naive v0 event timestamps must migrate as local time, not UTC.
375398
376399
The v0 schema stored the event ``timestamp`` column as a naive datetime in
377400
local time (``StorageEvent.from_event`` uses ``datetime.fromtimestamp`` and
378401
``to_event`` reads it back with naive ``.timestamp()``). Forcing UTC on that
379-
naive value shifted every migrated timestamp by the host's UTC offset. Pin a
380-
fixed non-UTC zone so the round trip is exact regardless of the host.
402+
naive value shifted every migrated timestamp by the host's UTC offset.
381403
"""
382-
monkeypatch.setenv("TZ", "Asia/Kolkata")
383-
time.tzset()
384-
try:
385-
original_epoch = 1000000.0
404+
original_epoch = 1000000.0
405+
406+
class NaiveLocalDatetime(datetime):
407+
"""Local naive datetime that rejects a timezone being forced onto it.
408+
409+
``replace`` and ``astimezone`` return instances of this subclass, so a
410+
migration that pins a timezone before reading the epoch back trips the
411+
guard even on a host whose local zone is already UTC and where the
412+
resulting epoch would be unchanged.
413+
"""
414+
415+
def timestamp(self) -> float:
416+
assert (
417+
self.tzinfo is None
418+
), f"migration forced {self.tzinfo} onto a naive v0 timestamp"
419+
return super().timestamp()
420+
421+
# The pinned zone is what catches a UTC recomputation that arrives by some
422+
# other route, e.g. calendar.timegm(), which the guard above cannot see.
423+
with _pinned_local_timezone("Asia/Kolkata"):
386424
# Exactly what v0.StorageEvent.from_event persisted: naive local time.
387-
naive_local_timestamp = datetime.fromtimestamp(original_epoch)
388-
assert naive_local_timestamp.tzinfo is None
425+
local = datetime.fromtimestamp(original_epoch)
426+
naive_local_timestamp = NaiveLocalDatetime(
427+
local.year,
428+
local.month,
429+
local.day,
430+
local.hour,
431+
local.minute,
432+
local.second,
433+
local.microsecond,
434+
)
389435

390436
event = mfsp._row_to_event({
391437
"id": "event-naive-timestamp",
@@ -396,8 +442,6 @@ def test_migrate_from_sqlalchemy_pickle_reads_naive_timestamp_as_local(
396442
})
397443

398444
assert event.timestamp == original_epoch
399-
finally:
400-
time.tzset()
401445

402446

403447
def test_migrate_from_sqlalchemy_pickle_blocks_unsafe_actions_pickle(
@@ -528,7 +572,7 @@ def __reduce__(self):
528572

529573

530574
def test_migrate_from_sqlalchemy_pickle_with_async_driver_urls(tmp_path):
531-
"""Tests that migration works with async driver URLs (fixes issue #4176).
575+
"""Tests that migration works with async driver URLs.
532576
533577
Users often provide async driver URLs (e.g., postgresql+asyncpg://) since
534578
that's what ADK requires at runtime. The migration tool should handle these
@@ -564,7 +608,7 @@ def test_migrate_from_sqlalchemy_pickle_with_async_driver_urls(tmp_path):
564608
source_session.commit()
565609
source_session.close()
566610

567-
# This should NOT raise an error about async drivers (the fix for #4176)
611+
# This should NOT raise an error about async drivers.
568612
mfsp.migrate(source_db_url, dest_db_url)
569613

570614
# Verify destination DB

tests/unittests/workflow/test_dynamic_node_scheduler.py

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
from google.adk.workflow._dynamic_node_scheduler import DynamicNodeScheduler
3030
from google.adk.workflow._dynamic_node_scheduler import DynamicNodeState
3131
from google.adk.workflow._node_state import NodeState
32-
from google.adk.workflow._node_status import NodeStatus
3332
from google.adk.workflow._workflow import _LoopState
3433
from pydantic import BaseModel
3534
from pydantic import ValidationError
@@ -568,20 +567,21 @@ async def _run_impl(self, *, ctx, node_input):
568567

569568

570569
def test_get_dynamic_tasks_excludes_done_tasks():
571-
"""get_dynamic_tasks should not return completed tasks (regression for #6082)."""
570+
"""get_dynamic_tasks should not return completed tasks."""
572571
import asyncio
573572

574573
loop = asyncio.new_event_loop()
574+
running_task = None
575575
try:
576576

577577
async def _done():
578578
return None
579579

580-
done_task = loop.run_until_complete(
581-
asyncio.ensure_future(_done(), loop=loop)
582-
)
583-
running_coro = asyncio.sleep(9999)
584-
running_task = loop.create_task(running_coro)
580+
# run_until_complete returns the coroutine's result, so the run entry has
581+
# to hold the task itself for the done-task filter to be exercised at all.
582+
done_task = loop.create_task(_done())
583+
loop.run_until_complete(done_task)
584+
running_task = loop.create_task(asyncio.sleep(9999))
585585

586586
state = DynamicNodeState()
587587
state.runs['path/done@r-1'] = DynamicNodeRun(
@@ -600,8 +600,14 @@ async def _done():
600600
tasks = state.get_dynamic_tasks()
601601

602602
assert tasks == [running_task]
603-
running_task.cancel()
604603
finally:
604+
# Cancelling without draining leaves the task pending at close() and the
605+
# sleep coroutine unawaited, which surfaces as a warning in later tests.
606+
if running_task is not None:
607+
running_task.cancel()
608+
loop.run_until_complete(
609+
asyncio.gather(running_task, return_exceptions=True)
610+
)
605611
loop.close()
606612

607613

0 commit comments

Comments
 (0)