Conversation
Greptile OverviewGreptile SummaryThis PR adds comprehensive E2E test coverage for the TypeScript SDK, including test suites for access tokens, basins, streams, metrics, and client lifecycle operations. Key Changes:
Test Implementation Quality:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as Test Suite
participant Env as S2Environment
participant S2 as S2 Client
participant API as S2 API
participant Cleanup as afterAll Hook
Note over Test,Cleanup: E2E Test Execution Flow
Test->>Env: parse() environment
alt Has S2_ACCESS_TOKEN
Env-->>Test: Returns config with accessToken
Test->>S2: new S2(config)
S2-->>Test: Client initialized
Test->>S2: Create test resources (basins/streams/tokens)
S2->>API: API calls
API-->>S2: Resources created
S2-->>Test: Success (setupOk = true)
Test->>Test: Execute test cases
Test->>S2: Perform operations
S2->>API: API calls
API-->>S2: Responses
S2-->>Test: Results
Test->>Cleanup: Trigger afterAll
Cleanup->>Cleanup: Check if (!s2) return
Cleanup->>S2: Delete resources
S2->>API: Cleanup calls
API-->>S2: Resources deleted
else No Access Token
Env-->>Test: Returns config without accessToken
Test->>Test: Early return (skip tests)
Test->>Cleanup: Trigger afterAll
Cleanup->>Cleanup: Guard: if (!s2) return
Cleanup-->>Test: Skip cleanup safely
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| createdBasins.push(basinA, basinB); | ||
| await waitForBasinReady(s2, basinA); | ||
| await waitForBasinReady(s2, basinB); | ||
| const basinClient = s2.basin(basinA); |
There was a problem hiding this comment.
[P1] afterAll can throw if beforeAll returned early (no access token) and s2 was never initialized.
When S2Environment.parse() yields no accessToken, beforeAll returns without setting s2, but afterAll still runs and calls s2.accessTokens.revoke(...), which will crash the suite with Cannot read properties of undefined.
Consider guarding cleanup with if (!s2) return; (or set setupOk/a created flag only after s2 is constructed).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/streamstore/src/tests/accessTokens.spec.e2e.test.ts
Line: 45:48
Comment:
[P1] `afterAll` can throw if `beforeAll` returned early (no access token) and `s2` was never initialized.
When `S2Environment.parse()` yields no `accessToken`, `beforeAll` returns without setting `s2`, but `afterAll` still runs and calls `s2.accessTokens.revoke(...)`, which will crash the suite with `Cannot read properties of undefined`.
Consider guarding cleanup with `if (!s2) return;` (or set `setupOk`/a `created` flag only after `s2` is constructed).
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const trackBasin = (name: string) => { | ||
| createdBasins.push(name); | ||
| return name; | ||
| }; | ||
|
|
There was a problem hiding this comment.
[P1] afterAll can crash when env token is missing because s2 is uninitialized.
beforeAll returns early if S2Environment.parse() has no accessToken, but afterAll still iterates createdBasins and calls s2.basins.delete(...). If the env var is set but parse fails (or token is omitted), this turns into a runtime error rather than a skipped suite. Guard cleanup with if (!s2) return; or only register cleanup after init succeeds.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/streamstore/src/tests/basins.spec.e2e.test.ts
Line: 21:26
Comment:
[P1] `afterAll` can crash when env token is missing because `s2` is uninitialized.
`beforeAll` returns early if `S2Environment.parse()` has no `accessToken`, but `afterAll` still iterates `createdBasins` and calls `s2.basins.delete(...)`. If the env var is set but parse fails (or token is omitted), this turns into a runtime error rather than a skipped suite. Guard cleanup with `if (!s2) return;` or only register cleanup after init succeeds.
How can I resolve this? If you propose a fix, please make it concise.| const end = Date.now(); | ||
| const start = end - hours * 60 * 60 * 1000; | ||
| return { start, end }; | ||
| }; | ||
|
|
||
| beforeAll(async () => { | ||
| const env = S2Environment.parse(); | ||
| if (!env.accessToken) return; | ||
| s2 = new S2(env as S2ClientOptions); | ||
| basinName = makeBasinName("ts-metrics"); | ||
| await s2.basins.create({ basin: basinName }); | ||
| await waitForBasinReady(s2, basinName); | ||
| const basin = s2.basin(basinName); | ||
| streamName = makeStreamName("metrics"); |
There was a problem hiding this comment.
[P1] afterAll can throw if beforeAll returned early (no access token) and s2 was never constructed.
This suite sets basinName/streamName after initializing s2, but afterAll only checks basinName and then calls s2.basins.delete(...). If env parsing returns no access token, s2 remains undefined and the cleanup will crash instead of no-op. Add if (!s2) return; or a created flag.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/streamstore/src/tests/metrics.spec.e2e.test.ts
Line: 20:33
Comment:
[P1] `afterAll` can throw if `beforeAll` returned early (no access token) and `s2` was never constructed.
This suite sets `basinName`/`streamName` after initializing `s2`, but `afterAll` only checks `basinName` and then calls `s2.basins.delete(...)`. If env parsing returns no access token, `s2` remains undefined and the cleanup will crash instead of no-op. Add `if (!s2) return;` or a `created` flag.
How can I resolve this? If you propose a fix, please make it concise.| await waitForBasinReady(s2, basinName); | ||
| basin = s2.basin(basinName); | ||
| }, TEST_TIMEOUT_MS); | ||
|
|
||
| afterAll(async () => { | ||
| if (basinName) { | ||
| await s2.basins.delete({ basin: basinName }).catch(() => {}); | ||
| } | ||
| }, TEST_TIMEOUT_MS); | ||
|
|
||
| describe("List streams", () => { | ||
| const listStreams: string[] = []; | ||
| let listPrefix = ""; | ||
|
|
There was a problem hiding this comment.
[P1] afterAll can crash if env parsing returns no access token and s2/basinName were never initialized.
beforeAll returns early on missing env.accessToken, but afterAll checks only basinName (declared but not initialized) and then uses s2.basins.delete(...). In that early-return scenario, s2 is undefined, so cleanup can throw. Guard with if (!s2) return; (or initialize basinName = "" like other suites).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/streamstore/src/tests/streams.spec.e2e.test.ts
Line: 42:55
Comment:
[P1] `afterAll` can crash if env parsing returns no access token and `s2`/`basinName` were never initialized.
`beforeAll` returns early on missing `env.accessToken`, but `afterAll` checks only `basinName` (declared but not initialized) and then uses `s2.basins.delete(...)`. In that early-return scenario, `s2` is undefined, so cleanup can throw. Guard with `if (!s2) return;` (or initialize `basinName = ""` like other suites).
How can I resolve this? If you propose a fix, please make it concise.| } finally { | ||
| await basin.streams.delete({ stream: streamName }).catch(() => {}); | ||
| } | ||
| }, | ||
| TEST_TIMEOUT_MS, | ||
| ); | ||
|
|
||
| it( | ||
| "clamps count to 1000 on unary read", |
There was a problem hiding this comment.
[P1] limited client may be constructed with endpoints: undefined, causing runtime failures unrelated to the behavior under test.
In the auto-prefix test, endpoints: S2Environment.parse().endpoints can be undefined (e.g., if endpoints are not set in the environment), which will likely throw inside new S2(...) or on first request. Since this suite already parsed env in beforeAll, reuse the stored env.endpoints (and/or assert it exists before running).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/streamstore/src/tests/spec.e2e.test.ts
Line: 123:131
Comment:
[P1] `limited` client may be constructed with `endpoints: undefined`, causing runtime failures unrelated to the behavior under test.
In the auto-prefix test, `endpoints: S2Environment.parse().endpoints` can be `undefined` (e.g., if endpoints are not set in the environment), which will likely throw inside `new S2(...)` or on first request. Since this suite already parsed env in `beforeAll`, reuse the stored `env.endpoints` (and/or assert it exists before running).
How can I resolve this? If you propose a fix, please make it concise.| `${prefix}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; | ||
|
|
||
| describeIf("Spec Integration Tests", () => { | ||
| let s2: S2; | ||
| let basinName: string; | ||
| let autoBasinName: string; | ||
| let basin: S2Basin; | ||
| let autoBasin: S2Basin; | ||
| let metricsStreamName: string; | ||
|
|
||
| const createStream = async (target: S2Basin, prefix: string) => { | ||
| const streamName = makeStreamName(prefix); | ||
| await target.streams.create({ stream: streamName }); | ||
| return streamName; |
There was a problem hiding this comment.
[P3] Duplicated helper logic and constants here drift from tests/helpers.ts.
This file redefines TEST_TIMEOUT_MS, makeBasinName, and makeStreamName instead of importing from ./helpers.js. That makes it easy for future changes (naming constraints/timeouts) to get out of sync across suites. Consider using the shared helpers to keep behavior consistent.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/streamstore/src/tests/spec.e2e.test.ts
Line: 18:31
Comment:
[P3] Duplicated helper logic and constants here drift from `tests/helpers.ts`.
This file redefines `TEST_TIMEOUT_MS`, `makeBasinName`, and `makeStreamName` instead of importing from `./helpers.js`. That makes it easy for future changes (naming constraints/timeouts) to get out of sync across suites. Consider using the shared helpers to keep behavior consistent.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
@greptile-apps review pls |
1 participant
based on specs from https://github.com/s2-streamstore/s2-sdk-go/tree/main/docs
credits: claude, codex