perf(nodejs): move the ingestion e2e suite to the parallel lane - #90032
Open
jose-sequeira wants to merge 6 commits into
Open
perf(nodejs): move the ingestion e2e suite to the parallel lane#90032jose-sequeira wants to merge 6 commits into
jose-sequeira wants to merge 6 commits into
Conversation
Contributor
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
jose-sequeira
force-pushed
the
jose-sequeira/e2e-global-setup
branch
from
August 27, 2026 12:06
001251b to
e7e1121
Compare
jose-sequeira
changed the title
jose-sequeira
changed the base branch from
nodejs-ingestion-topic-check-retry
to
master
August 27, 2026 12:07
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
jose-sequeira
marked this pull request as ready for review
August 27, 2026 12:59
Contributor
Prompt To Fix All With AI### Issue 1
nodejs/tests/helpers/ingestion-e2e.ts:528-529
**Concurrent tests share handlers**
When the two client-warnings cases run concurrently, each consumer connection overwrites the same module-scoped `mockCapturedHandler`, so a test can dispatch its batch through the other test's pipeline and dependencies, causing missing or unexpected ClickHouse rows and nondeterministic CI failures.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Merge branch 'master' into jose-sequeira..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+528
to
+529
| test.concurrent(name, async () => { | ||
| await ensureIngestionE2EInfraReady() |
Contributor
There was a problem hiding this comment.
When the two client-warnings cases run concurrently, each consumer connection overwrites the same module-scoped mockCapturedHandler, so a test can dispatch its batch through the other test's pipeline and dependencies, causing missing or unexpected ClickHouse rows and nondeterministic CI failures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodejs/tests/helpers/ingestion-e2e.ts
Line: 528-529
Comment:
**Concurrent tests share handlers**
When the two client-warnings cases run concurrently, each consumer connection overwrites the same module-scoped `mockCapturedHandler`, so a test can dispatch its batch through the other test's pipeline and dependencies, causing missing or unexpected ClickHouse rows and nondeterministic CI failures.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This was referenced Aug 27, 2026
The retry above lets its startup topic check ride out a transient metadata gap, which is what kept this suite on the serial lane. 152 tests, 29s on the serial lane. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tests are almost entirely idle, waiting on the ClickHouse Kafka engine to flush, so overlapping them is most of the suite's runtime. This needs the retry below. Each test builds its own KafkaProducerWrapper, so concurrency churns a producer per test through the broker, and the startup topic check used to fail on one absent metadata response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jest-circus starts test.concurrent bodies at the root block as soon as a nested beforeAll hits its first await. In the ingestion e2e suites that beforeAll creates the output topics and waits for the ClickHouse Kafka engine, so the first maxConcurrency tests raced topic creation and failed their startup topic check with a random subset of topics. Memoize the shared setup in the harness and await it inside every concurrent test, so readiness no longer depends on hook placement. Add a small test documenting the scheduling behavior.
…uites The four other ingestion e2e suites carried the same nested beforeAll setup. Route them through ensureIngestionE2EInfraReady so the setup lives in one place and no suite depends on hook placement.
The harness's test.concurrent bodies are wait-bound on ClickHouse's Kafka engine flush, not CPU-bound, so Jest's default of 5 in flight per worker leaves most of the file idle. 15 halves ingestion-e2e locally (31 s to 16 s); instrumenting the harness showed per-test infra setup is under 1% of the file, so sharing it would not have helped.
The suite already minted a team per test but ran them one at a time through module-level state (currentToken, a per-describe ingester). Porting it onto createTestWithTeamIngester removes that state and runs the 88 tests concurrently: 20 s to 5 s locally.
jose-sequeira
force-pushed
the
jose-sequeira/e2e-global-setup
branch
from
August 28, 2026 08:25
0438e8a to
ddb5497
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Anyone waiting on Node.js CI pays for
ingestion-e2ein the serial lane, where it is the single largest file (about 124 s of the roughly 550 s of serial work after #89152). Moving it to the parallel lane, with its tests running concurrently, cuts the slowest serial shard by about half.The move alone is flaky on a fresh broker, and #89866 handles that with a startup retry in the production
checkTopics. This PR takes the same move without touching production code, because the cause is in the test harness:test.concurrentbody at the root block as soon as a nestedbeforeAllhits its firstawait.beforeAllnested indescribe.each.maxConcurrencytests therefore start their consumer whileensureKafkaTopicsis still creating topics, and the startup topic check fails on whichever topics are not there yet.waitForClickHouseKafkaConsumerreturned, so even a passing topic check could lose events to a not-yet-assigned ClickHouse consumer.Changes
ingestion-e2enow runs in the parallel lane with its 152 tests concurrent.createTestWithTeamIngesterawaits a memoizedensureIngestionE2EInfraReady()inside every concurrent test, so topics and the ClickHouse consumer are ready before any consumer starts, regardless of hook placement.maxConcurrency: 15(Jest default 5). The setting only affectstest.concurrentbodies, which exist only in the shared harness. Those tests are wait-bound on ClickHouse's 500 ms Kafka engine flush, so more in flight halvesingestion-e2elocally (31 s to 16 s).person-updates-e2eruns on the same harness, so its 88 tests run concurrently instead of one at a time through module-level state (20 s to 5 s locally). Its bodies are unchanged; the diff is prettier re-indenting them inside the harness call.beforeAllinstead of carrying their own copy of the setup (mechanical).Note
The first two commits are #89866's rename and
test.concurrentchange, taken as-is. This PR replaces thecheckTopicsretry in that PR with the harness fix, so only one of the two should merge.How did you test this code?
Output topic verification failed for: tophog, with the retry disabled.createIngestionTestInfrais under 1% ofingestion-e2e(0.5 s across 152 tests), test bodies are 85%. That is why the change raises concurrency rather than sharing infra.maxConcurrencysweep locally: 5 → 31 s, 15 → 16 s, 30 → 12 s foringestion-e2e; 15 chosen as a first step. Three runs at 15 with all harness suites together passed.person-updates-e2ebefore and after the port, two runs each: 20.4 s / 19.3 s → 5.3 s / 4.3 s, 88 tests both ways.flag-evaluations-e2efails one assertion locally on the untouched perf(nodejs): run the ingestion e2e suite concurrently #89866 head as well (local ClickHouseflag_evaluationsschema) and passes in CI.hogli review(the Greptile CLI does not install in this environment) and the full Node.js suite.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Claude Fable 5) under @jose-sequeira's direction, starting from a review of #89866. Skills invoked:
/writing-pr-descriptions,/writing-tests. The mechanism was confirmed against the installed jest-circus 30.0.5 source and against the failing-test pattern on #89152's branch (always the first five tests in file order). The branch was first stacked on #89866 and then rebuilt on master without the retry commits, at the reviewer's request. SharingcreateIngestionTestInfraacross tests was measured and rejected as a non-lever before the concurrency change was made.