Skip to content

Commit accc40e

Browse files
mchwangclaude
andauthored
Fix flaky partial-request shutdown browser tests (#61)
Wait for the server's 'request' event before starting shutdown in the drain, partial-merge and partial-body tests, so the partial-request interleaving is ordered by events instead of a 10 ms timer. The in-flight question edit test now waits until the submission is intercepted, asserts the submitted body, and checks the durable note list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013owjkhQZhekCae7KP5rC3P
1 parent 5e97aa3 commit accc40e

1 file changed

Lines changed: 16 additions & 13 deletions

File tree

‎test/browser/review.spec.ts‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import { ReviewService } from '../../runner/review.ts';
1010
import { startServer } from '../../web/server.ts';
1111
import type { MergeGateway, MergeQueueGateway } from '../../github/merge.ts';
1212
let root: string, app: Awaited<ReturnType<typeof startServer>>;
13+
// Resolves once the server has parsed a request's headers and is reading its body, so a partial request is provably admitted before shutdown starts.
14+
function requestAdmitted(server: typeof app.server) { return new Promise<void>(resolve=>server.once('request',()=>resolve())); }
1315
function removeDemoOutOfScope(config: typeof app.service.config) { chmodSync(join(config.repository,'run.sh'),0o644);execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','Restore declared scope'],{cwd:config.repository,stdio:'pipe'}); }
1416
test.beforeEach(async () => { root=mkdtempSync(join(tmpdir(),'codeboost-browser-'));app=await startServer(createDemo(join(root,'demo')),0); });
1517
test.afterEach(async () => { await app.close();rmSync(root,{recursive:true,force:true}); });
@@ -404,15 +406,17 @@ for(const scenario of ['navigation','snapshot','removed item','failure']) test(`
404406
});
405407
for(const switchItem of [false,true]) test(`preserves edits made while a question submission is in flight (switch item: ${switchItem})`,async({page})=>{
406408
await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();
407-
let release!:()=>void;const held=new Promise<void>(resolve=>release=resolve);
408-
await page.route('**/api/action',async route=>{await held;await route.continue();});
409+
let release!:()=>void,arrived!:(body:string)=>void;const held=new Promise<void>(resolve=>release=resolve),submitted=new Promise<string>(resolve=>arrived=resolve);
410+
await page.route('**/api/action',async route=>{arrived(route.request().postData()??'');await held;await route.continue();});
409411
await page.getByLabel('Question about this item').fill('Submitted question');await page.getByRole('button',{name:'Ask agent',exact:true}).click();
412+
expect(JSON.parse(await submitted)).toMatchObject({action:'note',item:'P1',kind:'question',text:'Submitted question'});
410413
await page.getByLabel('Question about this item').fill('New unsent draft');
411414
if(switchItem){await page.getByRole('button',{name:/P2 Document retry behavior/}).click();await page.getByLabel('Question about this item').fill('Other item draft');}
412415
release();
413416
if(switchItem){await expect(page.locator('#saved')).toContainText('Question submitted');await expect(page.getByLabel('Question about this item')).toHaveValue('Other item draft');await page.getByRole('button',{name:/P1 Bound exponential retries/}).click();}
414417
await expect(page.getByText('Submitted question',{exact:true})).toBeVisible();
415418
await expect(page.getByLabel('Question about this item')).toHaveValue('New unsent draft');
419+
expect(app.service.store.getReviewNotes(app.service.config.identity).map(note=>note.text)).toEqual(['Submitted question']);
416420
});
417421
test('warns on completed answers when a snippet assignment changes',async({page})=>{
418422
const service=app.service,identity=service.config.identity,initial=service.load();
@@ -431,16 +435,16 @@ test('drains an in-flight question request before closing its agent manager',asy
431435
app=await startServer(config,0,(_prompt,signal)=>{calls++;return new Promise((_,reject)=>signal.addEventListener('abort',()=>reject(signal.reason),{once:true}));});
432436
const view=app.service.load(),body=JSON.stringify({action:'note',item:'P1',kind:'question',text:'Question during shutdown',token:view.token});
433437
const endpoint=new URL('/api/action',app.url);
434-
let response='';
438+
let response='',finish!:()=>void;const admitted=requestAdmitted(app.server);
435439
const completed=new Promise<void>((resolve,reject)=>{
436440
const req=httpRequest(endpoint,{method:'POST',headers:{'x-codeboost-token':app.token,'content-type':'application/json','content-length':Buffer.byteLength(body)}},res=>{
437441
res.setEncoding('utf8');res.on('data',chunk=>response+=chunk);res.on('end',resolve);
438442
});
439443
req.on('error',reject);req.write(body.slice(0,1));
440-
setTimeout(()=>req.end(body.slice(1)),50);
444+
finish=()=>req.end(body.slice(1));
441445
});
442-
await new Promise(resolve=>setTimeout(resolve,10));
443-
await Promise.all([app.close(),completed]);
446+
await Promise.race([admitted,completed]);const closing=app.close();finish();
447+
await Promise.all([closing,completed]);
444448
const reopened=new ReviewService(config);
445449
try {
446450
const note=reopened.load().notes.find(note=>note.text==='Question during shutdown');
@@ -471,23 +475,22 @@ test('aborts an admitted review status inspection after the shutdown drain',asyn
471475
});
472476
test('destroys a partial request body after the shutdown drain',async()=>{
473477
const config=app.service.config;await app.close();app=await startServer(config,0,undefined,undefined,50);
474-
const endpoint=new URL('/api/action',app.url),body=JSON.stringify({action:'note'});
475-
let admitted!:(value?:void)=>void;const requestAdmitted=new Promise<void>(resolve=>{admitted=resolve;});
478+
const endpoint=new URL('/api/action',app.url),body=JSON.stringify({action:'note'}),admitted=requestAdmitted(app.server);
476479
const completed=new Promise<'response'|'error'>(resolve=>{
477480
const req=httpRequest(endpoint,{method:'POST',headers:{'x-codeboost-token':app.token,'content-type':'application/json','content-length':Buffer.byteLength(body)}},res=>{res.resume();res.on('end',()=>resolve('response'));});
478-
req.on('error',()=>resolve('error'));req.write(body.slice(0,1),()=>admitted());
481+
req.on('error',()=>resolve('error'));req.write(body.slice(0,1));
479482
});
480-
await requestAdmitted;const started=Date.now();await app.close();expect(Date.now()-started).toBeLessThan(1000);expect(await completed).toBe('error');app=await startServer(config,0);
483+
await admitted;const started=Date.now();await app.close();expect(Date.now()-started).toBeLessThan(1000);expect(await completed).toBe('error');app=await startServer(config,0);
481484
});
482485
test('blocks a partially received merge request when shutdown starts',async()=>{
483486
const config={...app.service.config,demo:false};await app.close();let mergeCalls=0;let appRef:typeof app;
484487
const gateway:MergeGateway={inspect:async()=>{const snapshot=appRef.service.load().snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:false,requiredChecks:[],alreadyFixed:'clear'};},merge:async()=>{mergeCalls++;return {url:''};}};
485488
app=appRef=await startServer(config,0,undefined,gateway);let view=app.service.load();
486489
for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=app.service.act({action:'accept',key:segment.key,token:view.token});
487490
for(const item of view.items)view=app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token});
488-
const body=JSON.stringify({action:'merge',token:view.token}),endpoint=new URL('/api/action',app.url);let status=0,response='';
489-
const completed=new Promise<void>((resolve,reject)=>{const req=httpRequest(endpoint,{method:'POST',headers:{'x-codeboost-token':app.token,'content-type':'application/json','content-length':Buffer.byteLength(body)}},res=>{status=res.statusCode??0;res.setEncoding('utf8');res.on('data',chunk=>response+=chunk);res.on('end',resolve);});req.on('error',reject);req.write(body.slice(0,1));setTimeout(()=>req.end(body.slice(1)),50);});
490-
await new Promise(resolve=>setTimeout(resolve,10));await Promise.all([app.close(),completed]);
491+
const body=JSON.stringify({action:'merge',token:view.token}),endpoint=new URL('/api/action',app.url);let status=0,response='',finish!:()=>void;const admitted=requestAdmitted(app.server);
492+
const completed=new Promise<void>((resolve,reject)=>{const req=httpRequest(endpoint,{method:'POST',headers:{'x-codeboost-token':app.token,'content-type':'application/json','content-length':Buffer.byteLength(body)}},res=>{status=res.statusCode??0;res.setEncoding('utf8');res.on('data',chunk=>response+=chunk);res.on('end',resolve);});req.on('error',reject);req.write(body.slice(0,1));finish=()=>req.end(body.slice(1));});
493+
await Promise.race([admitted,completed]);const closing=app.close();finish();await Promise.all([closing,completed]);
491494
expect(status).toBe(503);expect(JSON.parse(response).error).toMatch(/shutting down/i);expect(mergeCalls).toBe(0);app=await startServer(config,0);
492495
});
493496
test('hides Retry until a timed-out invocation has actually settled',async({page})=>{

0 commit comments

Comments
 (0)