Repository navigation
refactor(lib): declarative toFinishWith, memoized RunTestResult, shared test registration - #207
Merged
Merged
Conversation
…ed test registration - toFinishWith evaluates a list of checks (lib/run-checks.ts) instead of hand-building diffs - shape matchers are table-driven; toBeNonEmptyObject no longer throws on null - RunTestResult memoizes promises (empty values and concurrent calls no longer refetch) - getDataset pages through items and toFinishWith counts via getDatasetItemCount, fixing #169 - testActor and testStandbyActor share defineActorTest Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEKaCaFK2UkrJS9oTw3Tg4
metalwarrior665
requested review from
JuanGalilea and
ruocco-l
as code owners
October 19, 2026 07:45
ruocco-l
reviewed
Oct 19, 2026
Comment on lines
+31
to
+34
| getStatistics = once(async (): Promise<SdkCrawlerStatistics | undefined> => { | ||
| const record = await this.getKeyValueStoreClient().getRecord('SDK_CRAWLER_STATISTICS_0'); | ||
| return record?.value as SdkCrawlerStatistics | undefined; | ||
| }); |
Contributor
There was a problem hiding this comment.
This will be obsolete in v4, might as well change it now #111
JuanGalilea
approved these changes
Oct 19, 2026
| toBeObject: ['an object', isObject], | ||
| toBeString: ['a string', (actual: unknown) => typeof actual === 'string'], | ||
| toBeWholeNumber: ['a whole number', (actual: unknown) => Number.isInteger(actual)], | ||
| } as const; |
Contributor
There was a problem hiding this comment.
at least plop a satisfies in there.
Member
Author
There was a problem hiding this comment.
Member
Author
|
Oops, this has been generated by testing a weekly routine (find best things to refactor & optimize) that I tried to see if Claude can catch up to @JuanGalilea. Will tell it to just do a draft |
…info count lags the run Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEKaCaFK2UkrJS9oTw3Tg4
There was a problem hiding this comment.
🟡 Changes recommended
Undefined numeric options regress existing behavior, and fixed-size dataset pages can still exceed Node’s string limit.
2 open issues
What changed in this PR
This PR refactors the testing library’s matchers, result fetching, and Actor test registration while aiming to preserve the public API.
Changes:
- Extracts declarative run checks and table-driven shape matchers.
- Memoizes result-fetching promises and adds dataset pagination to address #169.
- Shares Actor test registration logic and adds unit tests for checks, caching, and pagination.
| File | Description |
|---|---|
| test/unit/run-test-result.test.ts | Tests caching, retry behavior, and dataset pagination. |
| test/unit/run-checks.test.ts | Tests run expectations and charged-event checks. |
| lib/run-test-result.ts | Adds promise memoization and paginated dataset retrieval. |
| lib/run-checks.ts | Centralizes declarative run expectation evaluation. |
| lib/lib.ts | Shares registration logic between standard and standby Actor tests. |
| lib/extend-expect.ts | Uses table-driven shape matchers and extracted run checks. |
Review effort: Balanced
…ptions as disabled Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEKaCaFK2UkrJS9oTw3Tg4
metalwarrior665
pushed a commit
that referenced
this pull request
Oct 26, 2026
… undefined options as disabled Ports the fixes made in #207 onto this rewrite: - the dataset info's item count lags the run by 10-20 s, so toFinishWith counts the downloaded items again - page size 1000 - explicit undefined options disable their checks, as before - a failed lazy load is retried instead of cached Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEKaCaFK2UkrJS9oTw3Tg4
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
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.
What
A cleanup of the testing library (
lib/). The public API is unchanged.toFinishWithis declarative. The newlib/run-checks.tsturns the options into a list of{ name, pass, actual, expected }checks. This replaces about 130 lines of hand-built diff arrays and theisWithinIntervalfunction that mutated its arguments. Adding a check is now one entry in that list, and the matcher only formats the failures. The checks are unit-tested (test/unit/run-checks.test.ts). The existing matcher tests aredescribe.skipbecause they depend on a fixed run.toBeArray,toBeStringand the rest come from one table. This fixestoBeNonEmptyObjectthrowing onnull, the "a array" typo, and[object Object]in messages.RunTestResultmemoizes promises. The oldif (this.x) return this.xcaches refetched empty values (an empty log, a missingINPUT) and fetched twice when called concurrently. A smallonce()helper now shares one promise per getter and retries after a failure.getDataset()pages through items 10k at a time, so no single response can exceed Node's string limit.toFinishWithstill counts the downloaded items, because the dataset info's item count lags the run by 10-20 seconds. A comment inrun-checks.tssays so.testActorandtestStandbyActorsharedefineActorTest. That function holds the options merge, the name, theshouldRunrule and the soft-expectwiring, which were duplicated.Why not more
Several of the issues I looked at depend on decisions that aren't made yet, so I left them alone:
fails) already passes throughActorTestOptions, so it may just need docs.Verification
Type-check, lint, knip, prettier and
npm testpass.🤖 Generated with Claude Code
https://claude.ai/code/session_01PEKaCaFK2UkrJS9oTw3Tg4