Skip to content

Commit f1460dd

Browse files
Use active event-wait pattern in todos_changed E2E tests
Replaces the post-hoc events.filter() pattern with the existing getNextEventOfType helper (Node/Python) and a CompletableFuture event subscription (Java). This matches the pattern used by Go/.NET/Rust and removes a race window where the event could arrive after sendAndWait resolves but before the assertion is checked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent b5772fe commit f1460dd

3 files changed

Lines changed: 21 additions & 31 deletions

File tree

java/src/test/java/com/github/copilot/SessionTodosChangedTest.java

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,13 @@
66

77
import static org.junit.jupiter.api.Assertions.*;
88

9-
import java.util.ArrayList;
10-
import java.util.Collections;
11-
import java.util.List;
9+
import java.util.concurrent.CompletableFuture;
1210
import java.util.concurrent.TimeUnit;
1311

1412
import org.junit.jupiter.api.AfterAll;
1513
import org.junit.jupiter.api.BeforeAll;
1614
import org.junit.jupiter.api.Test;
1715

18-
import com.github.copilot.generated.SessionEvent;
1916
import com.github.copilot.generated.SessionTodosChangedEvent;
2017
import com.github.copilot.generated.rpc.PlanSqlTodoDependency;
2118
import com.github.copilot.rpc.MessageOptions;
@@ -46,8 +43,12 @@ void firesSessionTodosChangedAndExposesRowsAndDependencies() throws Exception {
4643
CopilotSession session = client
4744
.createSession(new SessionConfig().setOnPermissionRequest(PermissionHandler.APPROVE_ALL)).get();
4845

49-
List<SessionEvent> events = Collections.synchronizedList(new ArrayList<>());
50-
session.on(events::add);
46+
CompletableFuture<SessionTodosChangedEvent> todosChanged = new CompletableFuture<>();
47+
session.on(event -> {
48+
if (event instanceof SessionTodosChangedEvent todosEvent && !todosChanged.isDone()) {
49+
todosChanged.complete(todosEvent);
50+
}
51+
});
5152

5253
session.sendAndWait(new MessageOptions()
5354
.setPrompt("Use the sql tool to execute exactly these statements, in order, with no extra rows:\n"
@@ -57,14 +58,14 @@ void firesSessionTodosChangedAndExposesRowsAndDependencies() throws Exception {
5758
+ "Then stop. Do not insert any other rows or create any other tables."))
5859
.get(120, TimeUnit.SECONDS);
5960

60-
assertTrue(events.stream().anyMatch(SessionTodosChangedEvent.class::isInstance),
61+
assertNotNull(todosChanged.get(15, TimeUnit.SECONDS),
6162
"Should have received at least one session.todos_changed event");
6263

6364
var result = session.getRpc().plan.readSqlTodosWithDependencies().get(15, TimeUnit.SECONDS);
6465
assertEquals(2, result.rows().size());
6566
var ids = result.rows().stream().map(row -> row.id()).filter(id -> id != null).sorted().toList();
6667

67-
assertEquals(List.of("alpha", "beta"), ids);
68+
assertEquals(java.util.List.of("alpha", "beta"), ids);
6869
assertTrue(result.dependencies().stream().anyMatch(SessionTodosChangedTest::isBetaDependsOnAlpha),
6970
"Should contain beta -> alpha dependency");
7071

nodejs/test/e2e/session_todos_changed.e2e.test.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ import fs, { realpathSync } from "node:fs";
66
import os from "node:os";
77
import { join } from "node:path";
88
import { describe, expect, it } from "vitest";
9-
import { approveAll, type SessionEvent } from "../../src/index.js";
9+
import { approveAll } from "../../src/index.js";
1010
import { createSdkTestContext } from "./harness/sdkTestContext.js";
11+
import { getNextEventOfType } from "./harness/sdkTestHelper.js";
1112

1213
/**
1314
* E2E coverage for the runtime's `session.todos_changed` event and
@@ -28,10 +29,7 @@ describe("Todos changed event + readSqlTodosWithDependencies", async () => {
2829
async () => {
2930
const session = await client.createSession({ onPermissionRequest: approveAll });
3031

31-
const events: SessionEvent[] = [];
32-
session.on((event) => {
33-
events.push(event);
34-
});
32+
const todosChanged = getNextEventOfType(session, "session.todos_changed");
3533

3634
await session.sendAndWait({
3735
prompt:
@@ -42,8 +40,7 @@ describe("Todos changed event + readSqlTodosWithDependencies", async () => {
4240
"Then stop. Do not insert any other rows or create any other tables.",
4341
});
4442

45-
const todosEvents = events.filter((e) => e.type === "session.todos_changed");
46-
expect(todosEvents.length).toBeGreaterThanOrEqual(1);
43+
await todosChanged;
4744

4845
const result = await session.rpc.plan.readSqlTodosWithDependencies();
4946
const ids = result.rows

python/e2e/test_session_todos_changed_e2e.py

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22

33
from __future__ import annotations
44

5+
import asyncio
6+
57
import pytest
68

79
from copilot.session import PermissionHandler
810

9-
from .testharness import E2ETestContext
11+
from .testharness import E2ETestContext, get_next_event_of_type
1012

1113
pytestmark = pytest.mark.asyncio(loop_scope="module")
1214

@@ -18,28 +20,18 @@
1820
Then stop. Do not insert any other rows or create any other tables."""
1921

2022

21-
def _event_type_value(event) -> str:
22-
return getattr(event.type, "value", event.type)
23-
24-
2523
class TestSessionTodosChanged:
2624
async def test_fires_session_todos_changed_and_exposes_rows_and_dependencies(
2725
self, ctx: E2ETestContext
2826
):
2927
async with await ctx.client.create_session(
3028
on_permission_request=PermissionHandler.approve_all,
3129
) as session:
32-
events = []
33-
unsubscribe = session.on(events.append)
34-
try:
35-
await session.send_and_wait(PROMPT, timeout=120.0)
36-
finally:
37-
unsubscribe()
38-
39-
todos_events = [
40-
event for event in events if _event_type_value(event) == "session.todos_changed"
41-
]
42-
assert len(todos_events) >= 1
30+
todos_changed = asyncio.create_task(
31+
get_next_event_of_type(session, "session.todos_changed", timeout=120.0)
32+
)
33+
await session.send_and_wait(PROMPT, timeout=120.0)
34+
await todos_changed
4335

4436
result = await session.rpc.plan.read_sql_todos_with_dependencies()
4537
ids = sorted(row.id for row in result.rows if row.id)

0 commit comments

Comments
 (0)