Skip to content

Commit 34ab693

Browse files
jumskijumski
committed
chore: sort out lot of flaky CI issues (#366)
# Fix Supabase connectivity issues and improve test reliability ### TL;DR This PR addresses Supabase connectivity issues in CI by configuring Docker ulimits, improves test reliability with channel stabilization delays, and enhances article worker tests with proper mocking. ### What changed? - Added Docker daemon configuration in GitHub Actions to allow containers to set high ulimits (prevents "ulimit: open files" errors in Supabase containers) - Improved client tests by adding stabilization delay for realtime channels to fully establish routing - Refactored article flow worker tests to use proper mocking instead of real HTTP requests - Updated Claude bash command patterns in settings.json to use proper syntax - Simplified client project.json by removing redundant db:ensure target and integrating database setup directly into test commands - Added test-stability.sh script to verify channel stabilization reliability - Added separate vitest config for type checking that doesn't need global setup ### How to test? 1. Run client tests with `nx test client` to verify they pass consistently 2. Test article worker with `cd apps/demo/supabase/functions/article_flow_worker && deno test` 3. Run the stability test script: `cd pkgs/client && ./test-stability.sh 10` 4. Verify CI workflow completes successfully ### Why make this change? The changes address several reliability issues: 1. Supabase containers were failing in CI due to ulimit restrictions 2. Client tests were flaky due to race conditions in realtime channel subscriptions 3. Article worker tests were making real HTTP requests, making them brittle and slow 4. The test setup was unnecessarily complex with separate database preparation steps These improvements make the tests more reliable, faster, and less dependent on external services, resulting in more consistent CI runs and developer experience.
1 parent b25181f commit 34ab693

28 files changed

+673
-934
lines changed

‎.claude/settings.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,9 @@
3333
"Bash(tree:*)",
3434
"Bash(vim:*)",
3535
"Bash(test:*)",
36-
"Bash(rm:./.notes/scratch/*)",
37-
"Bash(mv:./.notes/*)",
38-
"Bash(cat:./.notes/*)",
36+
"Bash(rm ./.notes/scratch/:*)",
37+
"Bash(mv ./.notes/:*)",
38+
"Bash(cat ./.notes/:*)",
3939
"Bash(./.claude/skills/_shared/notes/search-notes.sh:*)",
4040
"Bash(./.claude/skills/_shared/notes/list-titles.sh:*)",
4141
"Bash(./.claude/skills/_shared/notes/list-topics.sh:*)",

‎.github/actions/setup/action.yml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,19 @@ description: 'Common setup steps for pgflow CI workflow (run after checkout)'
44
runs:
55
using: 'composite'
66
steps:
7+
# Configure Docker daemon to allow containers to set high ulimits
8+
# This prevents "ulimit: open files: cannot modify limit: Operation not permitted" errors
9+
# when Supabase containers (pooler, realtime) try to set RLIMIT_NOFILE to 100000.
10+
# The error occurs because GitHub Actions runners have restrictive ulimit defaults
11+
# that prevent containers from increasing their file descriptor limits.
12+
# See: https://github.com/orgs/supabase/discussions/18228
13+
- name: Configure Docker ulimits for Supabase
14+
shell: bash
15+
run: |
16+
sudo mkdir -p /etc/docker
17+
echo '{"default-ulimits":{"nofile":{"Name":"nofile","Hard":100000,"Soft":100000}}}' | sudo tee /etc/docker/daemon.json
18+
sudo systemctl restart docker
19+
720
- uses: pnpm/action-setup@v4
821
with:
922
version: '10.20.0'

‎apps/demo/supabase/functions/article_flow_worker/deno.lock‎

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎apps/demo/supabase/functions/article_flow_worker/tasks/summarize-article.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,10 @@ async function summarizeWithGroq(content: string, apiKey: string) {
4040
},
4141
{
4242
role: 'user',
43-
content: `Summarize this article in 2-3 sentences and determine its sentiment:\n\n${content.slice(0, 4000)}`
43+
content: `Summarize this article in 2-3 sentences and determine its sentiment:\n\n${content.slice(
44+
0,
45+
4000
46+
)}`
4447
}
4548
],
4649
temperature: 0.7,
@@ -84,7 +87,10 @@ async function summarizeWithOpenAI(content: string, apiKey: string) {
8487
},
8588
{
8689
role: 'user',
87-
content: `Summarize this article in 2-3 sentences and determine its sentiment:\n\n${content.slice(0, 4000)}`
90+
content: `Summarize this article in 2-3 sentences and determine its sentiment:\n\n${content.slice(
91+
0,
92+
4000
93+
)}`
8894
}
8995
],
9096
temperature: 0.7,

‎apps/demo/supabase/functions/article_flow_worker/tests/fetch-article.test.ts‎

Lines changed: 90 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { assert } from 'https://deno.land/std@0.208.0/assert/mod.ts';
2+
import { stub } from 'https://deno.land/std@0.208.0/testing/mock.ts';
23
import { fetchArticle } from '../tasks/fetch-article.ts';
34
import { load } from 'https://deno.land/std@0.208.0/dotenv/mod.ts';
45

@@ -7,47 +8,113 @@ await load({ envPath: '../.env', export: true }).catch(() => {
78
console.log('No .env file found, using environment variables');
89
});
910

10-
Deno.test('fetchArticle - fetches real article from Hacker News', async () => {
11-
// Use a stable HN article URL that should always exist
12-
const url = 'https://news.ycombinator.com/item?id=35629516';
11+
const mockJinaResponse = `# Mock Article Title
1312
14-
const result = await fetchArticle(url);
13+
This is mock content with enough text to pass validation tests.
14+
It has multiple lines and is over 100 characters long.
15+
`;
1516

16-
// Verify we got a result with both content and title
17-
assert(result.content, 'Should have content');
18-
assert(result.title, 'Should have title');
19-
assert(result.content.length > 100, 'Content should be substantial');
20-
assert(result.title !== 'Untitled Article', 'Should extract a real title');
17+
function stubFetch(response: { status: number; statusText: string; body?: string }) {
18+
return stub(globalThis, 'fetch', () =>
19+
Promise.resolve(
20+
new Response(response.body || '', {
21+
status: response.status,
22+
statusText: response.statusText
23+
})
24+
)
25+
);
26+
}
2127

22-
console.log(`✓ Fetched article: "${result.title}" (${result.content.length} chars)`);
23-
});
28+
function stubFetchError(error: Error) {
29+
return stub(globalThis, 'fetch', () => Promise.reject(error));
30+
}
2431

25-
Deno.test('fetchArticle - fetches real article from TechCrunch', async () => {
26-
// Use TechCrunch homepage which should always work
27-
const url = 'https://techcrunch.com';
32+
// Mocked tests (always run)
33+
Deno.test('fetchArticle - fetches article with mocked fetch', async () => {
34+
const fetchStub = stubFetch({ status: 200, statusText: 'OK', body: mockJinaResponse });
2835

29-
const result = await fetchArticle(url);
36+
try {
37+
const url = 'https://example.com/article';
38+
const result = await fetchArticle(url);
3039

31-
assert(result.content, 'Should have content');
32-
assert(result.title, 'Should have title');
33-
assert(result.content.length > 100, 'Content should be substantial');
40+
assert(result.content, 'Should have content');
41+
assert(result.title, 'Should have title');
42+
assert(result.content.length > 100, 'Content should be substantial');
43+
assert(result.title === 'Mock Article Title', 'Should extract title from mock response');
3444

35-
console.log(`✓ Fetched article: "${result.title}" (${result.content.length} chars)`);
45+
console.log('✓ Mock fetch works');
46+
} finally {
47+
fetchStub.restore();
48+
}
49+
});
50+
51+
Deno.test('fetchArticle - handles non-OK response with mocked fetch', async () => {
52+
const fetchStub = stubFetch({
53+
status: 451,
54+
statusText: 'Unavailable For Legal Reasons',
55+
body: 'Unavailable'
56+
});
57+
58+
try {
59+
const url = 'https://example.com/blocked-article';
60+
await fetchArticle(url);
61+
assert(false, 'Should have thrown an error');
62+
} catch (error) {
63+
assert(error instanceof Error);
64+
assert(error.message.includes('Failed to fetch'));
65+
assert(error.message.includes('451'));
66+
console.log('✓ Properly handles non-OK responses');
67+
} finally {
68+
fetchStub.restore();
69+
}
3670
});
3771

3872
Deno.test({
39-
name: 'fetchArticle - handles non-existent URL gracefully',
40-
sanitizeResources: false, // Disable resource leak check for this test
73+
name: 'fetchArticle - handles network errors with mocked fetch',
74+
sanitizeResources: false,
4175
fn: async () => {
42-
const url = 'https://this-domain-definitely-does-not-exist-12345.com/article';
76+
const fetchStub = stubFetchError(new TypeError('Network error'));
4377

4478
try {
79+
const url = 'https://example.com/article';
4580
await fetchArticle(url);
4681
assert(false, 'Should have thrown an error');
4782
} catch (error) {
4883
assert(error instanceof Error);
4984
assert(error.message.includes('Failed to fetch'));
50-
console.log('✓ Properly handles fetch errors');
85+
console.log('✓ Properly handles network errors');
86+
} finally {
87+
fetchStub.restore();
5188
}
5289
}
5390
});
91+
92+
// Real HTTP tests (only run when USE_REAL_HTTP=true)
93+
if (Deno.env.get('USE_REAL_HTTP') === 'true') {
94+
Deno.test('fetchArticle - fetches real article from Hacker News', async () => {
95+
const url = 'https://news.ycombinator.com/item?id=35629516';
96+
97+
const result = await fetchArticle(url);
98+
99+
assert(result.content, 'Should have content');
100+
assert(result.title, 'Should have title');
101+
assert(result.content.length > 100, 'Content should be substantial');
102+
assert(result.title !== 'Untitled Article', 'Should extract a real title');
103+
104+
console.log(`✓ Fetched real article: "${result.title}" (${result.content.length} chars)`);
105+
});
106+
107+
Deno.test('fetchArticle - fetches real article from TechCrunch', async () => {
108+
const url = 'https://techcrunch.com';
109+
110+
const result = await fetchArticle(url);
111+
112+
assert(result.content, 'Should have content');
113+
assert(result.title, 'Should have title');
114+
assert(result.content.length > 100, 'Content should be substantial');
115+
116+
console.log(`✓ Fetched real article: "${result.title}" (${result.content.length} chars)`);
117+
});
118+
} else {
119+
console.log('ℹ Skipping real HTTP tests (set USE_REAL_HTTP=true to run them)');
120+
}

‎package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
"eslint-config-prettier": "10.1.5",
3939
"jiti": "2.4.2",
4040
"jsdom": "~22.1.0",
41+
"jsonc-eslint-parser": "^2.4.1",
4142
"jsr": "^0.13.4",
4243
"netlify-cli": "^22.1.3",
4344
"nx": "21.2.1",

‎pkgs/client/__tests__/PgflowClient.test.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
createRunResponse,
88
mockRpcCall,
99
emitBroadcastEvent,
10+
createSyncSchedule,
1011
} from './helpers/test-utils';
1112
import {
1213
createRunCompletedEvent,
@@ -24,7 +25,7 @@ describe('PgflowClient', () => {
2425

2526
test('initializes correctly', () => {
2627
const { client } = createMockClient();
27-
const pgflowClient = new PgflowClient(client);
28+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
2829

2930
expect(pgflowClient).toBeDefined();
3031
});
@@ -40,7 +41,7 @@ describe('PgflowClient', () => {
4041
);
4142
mockRpcCall(mocks, response);
4243

43-
const pgflowClient = new PgflowClient(client);
44+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
4445
const run = await pgflowClient.startFlow(FLOW_SLUG, input);
4546

4647
// Check RPC call
@@ -66,7 +67,7 @@ describe('PgflowClient', () => {
6667
const error = new Error('RPC error');
6768
mockRpcCall(mocks, { data: null, error });
6869

69-
const pgflowClient = new PgflowClient(client);
70+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
7071

7172
// The startFlow call should reject with the error
7273
await expect(
@@ -84,7 +85,7 @@ describe('PgflowClient', () => {
8485
);
8586
mockRpcCall(mocks, response);
8687

87-
const pgflowClient = new PgflowClient(client);
88+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
8889

8990
// First call should fetch from DB
9091
const run1 = await pgflowClient.getRun(RUN_ID);
@@ -105,7 +106,7 @@ describe('PgflowClient', () => {
105106
// Mock the RPC call to return no run
106107
mockRpcCall(mocks, { data: { run: null, steps: [] }, error: null });
107108

108-
const pgflowClient = new PgflowClient(client);
109+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
109110

110111
const result = await pgflowClient.getRun('nonexistent-id');
111112

@@ -120,7 +121,7 @@ describe('PgflowClient', () => {
120121
mockRpcCall(mocks, response);
121122

122123
// Create test client
123-
const pgflowClient = new PgflowClient(client);
124+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
124125

125126
// Get a run to create an instance
126127
const run = await pgflowClient.getRun(RUN_ID);
@@ -154,7 +155,7 @@ describe('PgflowClient', () => {
154155
const response = createRunResponse({ run_id: RUN_ID, input });
155156
mockRpcCall(mocks, response);
156157

157-
const pgflowClient = new PgflowClient(client);
158+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
158159

159160
// Start a flow to create a run instance
160161
const run = await pgflowClient.startFlow(FLOW_SLUG, input);
@@ -189,7 +190,7 @@ describe('PgflowClient', () => {
189190
mockRpcCall(mocks, response1);
190191
mockRpcCall(mocks, response2);
191192

192-
const pgflowClient = new PgflowClient(client);
193+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
193194

194195
// Get two different runs
195196
await pgflowClient.getRun('1');
@@ -215,7 +216,7 @@ describe('PgflowClient', () => {
215216
const response = createRunResponse({ run_id: RUN_ID, input }, []);
216217
mockRpcCall(mocks, response);
217218

218-
const pgflowClient = new PgflowClient(client);
219+
const pgflowClient = new PgflowClient(client, { realtimeStabilizationDelayMs: 0, schedule: createSyncSchedule() });
219220

220221
// Start a flow
221222
const run = await pgflowClient.startFlow(FLOW_SLUG, input);

‎pkgs/client/__tests__/SupabaseBroadcastAdapter.simple.test.ts‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,15 @@ describe('SupabaseBroadcastAdapter - Simple Tests', () => {
3737
*/
3838
test('subscribes to a run and configures channel correctly', async () => {
3939
const { client, mocks } = createMockClient();
40-
const adapter = new SupabaseBroadcastAdapter(client);
40+
const adapter = new SupabaseBroadcastAdapter(client, { stabilizationDelayMs: 0 });
4141

4242
// Setup realistic channel subscription
4343
mockChannelSubscription(mocks);
4444

45-
// Subscribe to run
46-
await adapter.subscribeToRun(RUN_ID);
45+
// Subscribe to run (need to advance timers for setTimeout(..., 0))
46+
const subscribePromise = adapter.subscribeToRun(RUN_ID);
47+
await vi.runAllTimersAsync();
48+
await subscribePromise;
4749

4850
// Check channel was created with correct name
4951
expect(client.channel).toHaveBeenCalledWith(`pgflow:run:${RUN_ID}`);
@@ -71,7 +73,7 @@ describe('SupabaseBroadcastAdapter - Simple Tests', () => {
7173
*/
7274
test('properly routes events to registered callbacks', () => {
7375
const { client, mocks } = createMockClient();
74-
const adapter = new SupabaseBroadcastAdapter(client);
76+
const adapter = new SupabaseBroadcastAdapter(client, { stabilizationDelayMs: 0 });
7577

7678
// Set up event listeners
7779
const runSpy = vi.fn();
@@ -112,7 +114,7 @@ describe('SupabaseBroadcastAdapter - Simple Tests', () => {
112114
error: null,
113115
});
114116

115-
const adapter = new SupabaseBroadcastAdapter(client);
117+
const adapter = new SupabaseBroadcastAdapter(client, { stabilizationDelayMs: 0 });
116118

117119
// Call method directly
118120
const result = await adapter.getRunWithStates(RUN_ID);
@@ -135,13 +137,15 @@ describe('SupabaseBroadcastAdapter - Simple Tests', () => {
135137
*/
136138
test('properly cleans up on unsubscribe', async () => {
137139
const { client, mocks } = createMockClient();
138-
const adapter = new SupabaseBroadcastAdapter(client);
140+
const adapter = new SupabaseBroadcastAdapter(client, { stabilizationDelayMs: 0 });
139141

140142
// Setup realistic channel subscription
141143
mockChannelSubscription(mocks);
142144

143-
// Subscribe then unsubscribe
144-
await adapter.subscribeToRun(RUN_ID);
145+
// Subscribe then unsubscribe (need to advance timers for setTimeout(..., 0))
146+
const subscribePromise = adapter.subscribeToRun(RUN_ID);
147+
await vi.runAllTimersAsync();
148+
await subscribePromise;
145149
adapter.unsubscribe(RUN_ID);
146150

147151
// Check channel was unsubscribed

0 commit comments

Comments
 (0)