From 54f744eb0ba78451d78580c4b57295a3cd710678 Mon Sep 17 00:00:00 2001 From: Jan Buchar Date: Thu, 24 Sep 2026 11:27:41 +0200 Subject: [PATCH] refactor: Fix tests that depended on private methods (#4150) --- docs/public-api/crawlee-basic.api.md | 1 + .../autoscaling/concurrency_system.ts | 3 +- .../src/internals/basic-crawler.ts | 8 +- .../internals/throttling_request_manager.ts | 35 ++- .../test/throttling_request_manager.test.ts | 100 ++++---- .../src/internals/browser-crawler.ts | 1 + packages/browser-pool/src/browser-pool.ts | 16 +- ...fferent-client-should-be-respected.test.ts | 12 +- .../src/internals/http-crawler.ts | 47 ++-- scripts/oxlint-plugin.test.ts | 2 + scripts/oxlint-plugin.ts | 4 + test/browser-pool/browser-pool.test.ts | 220 ++++++++---------- test/core/autoscaling/autoscaled_pool.test.ts | 175 ++++++++------ .../autoscaling/concurrency_system.test.ts | 72 +++--- test/core/autoscaling/snapshotter.test.ts | 2 +- .../playwright_launcher.test.ts | 27 +-- .../puppeteer_launcher.test.ts | 5 +- .../adaptive_playwright_crawler.test.ts | 17 +- test/core/crawlers/basic_crawler.test.ts | 137 +++++++---- test/core/crawlers/browser_crawler.test.ts | 5 +- test/core/crawlers/cheerio_crawler.test.ts | 87 ++++--- test/core/crawlers/statistics.test.ts | 26 ++- test/core/impit_http_client.test.ts | 12 +- test/core/request_list.test.ts | 56 +++-- test/core/session_pool/session_pool.test.ts | 14 +- test/core/storages/request_queue.test.ts | 40 ++-- .../core/storages/storage_transaction.test.ts | 25 +- .../stagehand-controller.test.ts | 35 +-- 28 files changed, 650 insertions(+), 534 deletions(-) diff --git a/docs/public-api/crawlee-basic.api.md b/docs/public-api/crawlee-basic.api.md index 235618f5ae5a..60fb0fa96ddb 100644 --- a/docs/public-api/crawlee-basic.api.md +++ b/docs/public-api/crawlee-basic.api.md @@ -221,6 +221,7 @@ export class ConcurrencySystem implements IConcurrencySystem { get isRunning(): boolean; get maxConcurrency(): number; set maxConcurrency(value: number); + readonly maxTasksPerMinute: number; get minConcurrency(): number; set minConcurrency(value: number); registerTaskEnd(_consumer?: ConcurrencyConsumer): void; diff --git a/packages/basic-crawler/src/internals/autoscaling/concurrency_system.ts b/packages/basic-crawler/src/internals/autoscaling/concurrency_system.ts index d413528214d0..b89fad6c8406 100644 --- a/packages/basic-crawler/src/internals/autoscaling/concurrency_system.ts +++ b/packages/basic-crawler/src/internals/autoscaling/concurrency_system.ts @@ -223,7 +223,8 @@ export class ConcurrencySystem implements IConcurrencySystem { private readonly scaleDownStepRatio: number; readonly #loggingIntervalMillis: number; readonly #autoscaleIntervalMillis: number; - private readonly maxTasksPerMinute: number; + /** The cap on tasks started per minute, or `Infinity` when uncapped. */ + readonly maxTasksPerMinute: number; #minConcurrency: number; #maxConcurrency: number; diff --git a/packages/basic-crawler/src/internals/basic-crawler.ts b/packages/basic-crawler/src/internals/basic-crawler.ts index 883eaefed097..8d515afbca6d 100644 --- a/packages/basic-crawler/src/internals/basic-crawler.ts +++ b/packages/basic-crawler/src/internals/basic-crawler.ts @@ -706,8 +706,7 @@ export class BasicCrawler< * request queue; subsequent ones get their own queue via a unique alias so they don't * collide. */ - // kept as TS-private: tests reset the counter at runtime - private static instanceCount = 0; + static #instanceCount = 0; /** * Tracks crawler instances that accessed shared state without having an explicit id. @@ -1066,7 +1065,7 @@ export class BasicCrawler< // Initialize the Configuration instance to avoid lazy loading in the components serviceLocator.getConfiguration(); - const instanceIndex = BasicCrawler.instanceCount++; + const instanceIndex = BasicCrawler.#instanceCount++; this.#identity = { instanceIndex, hasExplicitId: id !== undefined, id: id ?? String(instanceIndex) }; if (requestManager !== undefined && (requestList !== undefined || requestQueue !== undefined)) { @@ -2637,6 +2636,7 @@ export class BasicCrawler< } /** Handles a single request - runs the request handler with retries, error handling, and lifecycle management. */ + // oxlint-disable-next-line crawlee/prefer-private-fields -- patched by @crawlee/otel private async handleRequest( crawlingContext: ExtendedContext, requestSource: IRequestManager, @@ -2817,6 +2817,7 @@ export class BasicCrawler< * * @param request The request object, passed separately to circumvent potential dynamic logic in crawlingContext.request */ + // oxlint-disable-next-line crawlee/prefer-private-fields -- patched by @crawlee/otel private async requestFunctionErrorHandler( error: Error, crawlingContext: CrawlingContext, @@ -2895,6 +2896,7 @@ export class BasicCrawler< await this.handleFailedRequestHandler(crawlingContext, error); // This function prints an error message. } + // oxlint-disable-next-line crawlee/prefer-private-fields -- patched by @crawlee/otel private async handleFailedRequestHandler(crawlingContext: CrawlingContext, error: Error): Promise { // Always log the last error regardless if the user provided a failedRequestHandler const { id, url, method, uniqueKey } = crawlingContext.request; diff --git a/packages/basic-crawler/src/internals/throttling_request_manager.ts b/packages/basic-crawler/src/internals/throttling_request_manager.ts index 1c753b04b5c7..a88f54bb3e61 100644 --- a/packages/basic-crawler/src/internals/throttling_request_manager.ts +++ b/packages/basic-crawler/src/internals/throttling_request_manager.ts @@ -279,9 +279,8 @@ export class ThrottlingRequestManager>(); - // Not `#private`, unlike the rest: the tests reach for these two. - private readonly domainStates = new Map(); - private readonly log: CrawleeLogger; + readonly #domainStates = new Map(); + readonly #log: CrawleeLogger; /** Domains from the `domains` option, which are throttled whether or not the crawl ever visits them. */ readonly #listedDomains = new Set(); @@ -355,7 +354,7 @@ export class ThrottlingRequestManager now >= throttledUntil(state) && this.#subManagers.has(state.domain)) .sort((a, b) => throttledUntil(a) - throttledUntil(b)) .map((state) => state.domain); @@ -642,7 +641,7 @@ export class ThrottlingRequestManager this.#maxDelayMs) { const source = waitGiven ? 'requested wait' : 'exponential backoff'; - this.log.warning( + this.#log.warning( `Capping ${source} delay of ${(delayMs / 1000).toFixed(1)}s for domain "${state.domain}" ` + `to maxDelaySecs (${(this.#maxDelayMs / 1000).toFixed(1)}s); the domain may continue to rate-limit. ` + `Consider increasing maxDelaySecs if this recurs.`, @@ -730,7 +729,7 @@ export class ThrottlingRequestManager { serviceLocator.setStorageBackend(new MemoryStorageBackend()); }); + afterEach(() => { + vitest.useRealTimers(); + }); + async function createQueue(name = 'inner-queue') { return RequestQueue.open({ name }); } - function domainState(manager: ThrottlingRequestManager, domain: string) { - return (manager as any).domainStates.get(domain); + /** + * Freezes `Date`, the manager's only clock, so a test can jump past a backoff instead of sleeping it out - + * a loaded CI box cannot race that. Timers stay real, so storage and `sleep` behave as usual. + */ + function freezeClock() { + vitest.useFakeTimers({ toFake: ['Date'] }); + } + + function advanceClock(ms: number) { + vitest.setSystemTime(Date.now() + ms); + } + + async function readyAt(manager: ThrottlingRequestManager) { + const status = await manager.checkReadiness(); + return status.status === 'waiting' ? status.readyAt : undefined; } /** Models the crawler's task loop: poll, and idle while the manager reports itself empty. */ @@ -215,11 +232,16 @@ describe('ThrottlingRequestManager', () => { }); test('warns that requestsFromUrl sources cannot be domain-routed', async () => { + // The manager logs through a `child()` of the shared logger; collapsing `child` onto the + // logger itself is the only way to see what it emits. + const logger = serviceLocator.getLogger(); + vitest.spyOn(logger, 'child').mockReturnValue(logger); + const warning = vitest.spyOn(logger, 'warning').mockImplementation(() => {}); + const manager = new ThrottlingRequestManager({ inner: await createQueue(), domains: ['example.com'], }); - const warning = vitest.spyOn((manager as any).log, 'warning').mockImplementation(() => {}); await manager.addRequestsBatched([ { requestsFromUrl: 'https://example.com/urls.txt' }, @@ -488,7 +510,7 @@ describe('ThrottlingRequestManager', () => { await manager.addRequest({ url: 'https://example.com/1' }); await manager.addRequest({ url: 'https://foo.com/1' }); - // Long enough that a loaded box cannot race the assertions below - the backoff is zeroed, never waited out. + // Long enough that a loaded box cannot race the assertions below - the clock jumps past it, never waits. expect( manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited', waitMs: 10_000 }), ).toBe(true); @@ -503,7 +525,7 @@ describe('ThrottlingRequestManager', () => { expect(waiting).toMatchObject({ status: 'waiting' }); expect(waiting.status === 'waiting' && waiting.readyAt).toBeGreaterThan(Date.now()); - domainState(manager, 'example.com').backoffUntil = 0; + advanceClock(10_000); expect((await manager.fetchNextRequest())!.url).toBe('https://example.com/1'); }); @@ -514,13 +536,16 @@ describe('ThrottlingRequestManager', () => { baseDelaySecs: 0.05, maxDelaySecs: 60, }); + await manager.addRequest({ url: 'https://example.com/1' }); + freezeClock(); // Eight requests were already in flight when the limit was hit; they all come back 429. for (let i = 0; i < 8; i++) { expect(manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' })).toBe(true); } - expect(domainState(manager, 'example.com').consecutive429Count).toBe(1); + // The first step of the backoff, not the eighth. + expect(await readyAt(manager)).toBe(Date.now() + 50); }); test('the backoff decays once the domain has stopped rate-limiting', async () => { @@ -530,25 +555,20 @@ describe('ThrottlingRequestManager', () => { baseDelaySecs: 10, maxDelaySecs: 60, }); - const state = domainState(manager, 'example.com'); + await manager.addRequest({ url: 'https://example.com/1' }); + freezeClock(); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - // Rewinding both clocks beats sleeping out real delays - a loaded CI box cannot race it. - const rewind = (ms: number) => { - state.backoffUntil -= ms; - state.backoffDecaysAt -= ms; - }; - // Past the backoff but still inside the decay window: the next 429 continues the same burst. - rewind(11_000); + advanceClock(11_000); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - expect(state.consecutive429Count).toBe(2); + expect(await readyAt(manager)).toBe(Date.now() + 20_000); // Past the decay window as well: the domain is treated as recovered and the exponent restarts. - rewind(41_000); + advanceClock(41_000); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - expect(state.consecutive429Count).toBe(1); + expect(await readyAt(manager)).toBe(Date.now() + 10_000); }); test('caps the delay at maxDelaySecs', async () => { @@ -557,10 +577,11 @@ describe('ThrottlingRequestManager', () => { domains: ['example.com'], maxDelaySecs: 1, }); + await manager.addRequest({ url: 'https://example.com/1' }); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited', waitMs: 3_600_000 }); - expect(domainState(manager, 'example.com').backoffUntil).toBeLessThanOrEqual(Date.now() + 1000); + expect(await readyAt(manager)).toBeLessThanOrEqual(Date.now() + 1000); }); test('fetchNextRequest does not block while a domain is throttled', async () => { @@ -625,6 +646,10 @@ describe('ThrottlingRequestManager', () => { }); describe('stall detection', () => { + beforeEach(() => { + freezeClock(); + }); + const stallingManager = async () => new ThrottlingRequestManager({ inner: await createQueue(), @@ -633,12 +658,10 @@ describe('ThrottlingRequestManager', () => { maxDomainStallSecs: 30, }); - /** - * Ages the domain's ongoing run of 429s past the stall threshold. Backdating beats sleeping - a loaded - * CI box cannot race it. - */ - const stallFor = (manager: ThrottlingRequestManager, domain: string) => { - domainState(manager, domain).rateLimitedSince -= 60_000; + /** The domain is still refusing once the stall window has gone by. */ + const stallFor = (manager: ThrottlingRequestManager, url: string) => { + advanceClock(31_000); + manager.recordPacingSignal({ url, reason: 'rateLimited' }); }; test('gives up on a domain that never lets a request through', async () => { @@ -648,7 +671,7 @@ describe('ThrottlingRequestManager', () => { expect((await manager.checkReadiness()).status).not.toBe('stalled'); - stallFor(manager, 'example.com'); + stallFor(manager, 'https://example.com/1'); expect(await manager.checkReadiness()).toMatchObject({ status: 'stalled', reason: expect.stringContaining('example.com'), @@ -661,10 +684,10 @@ describe('ThrottlingRequestManager', () => { const manager = await stallingManager(); await manager.addRequest({ url: 'https://example.com/1' }); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - stallFor(manager, 'example.com'); + stallFor(manager, 'https://example.com/1'); // Its backoff has run out, so its own queue would happily hand the request over. - domainState(manager, 'example.com').backoffUntil = 0; + advanceClock(1_000); const subQueue = await RequestQueue.open({ alias: 'throttled-example.com' }); await expect(subQueue.checkReadiness()).resolves.toEqual({ status: 'ready' }); @@ -675,7 +698,7 @@ describe('ThrottlingRequestManager', () => { const manager = await stallingManager(); await manager.addRequest({ url: 'https://example.com/1' }); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - stallFor(manager, 'example.com'); + stallFor(manager, 'https://example.com/1'); await manager.addRequest({ url: 'https://other.com/1' }); @@ -687,9 +710,10 @@ describe('ThrottlingRequestManager', () => { await manager.addRequest({ url: 'https://example.com/1' }); await manager.addRequest({ url: 'https://example.com/2' }); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - stallFor(manager, 'example.com'); + stallFor(manager, 'https://example.com/1'); + advanceClock(1_000); - await manager.markRequestAsHandled((await pollForNextRequest(manager))!); + await manager.markRequestAsHandled((await manager.fetchNextRequest())!); expect((await manager.checkReadiness()).status).not.toBe('stalled'); }); @@ -711,7 +735,7 @@ describe('ThrottlingRequestManager', () => { const request = (await manager.fetchNextRequest())!; manager.recordPacingSignal({ url: request.url, reason: 'rateLimited' }); await manager.reclaimRequest(request); - stallFor(manager, 'example.com'); + stallFor(manager, 'https://example.com/1'); // Sweeping it is what finds the request; without it the crawl would wait on the domain forever. await manager.fetchNextRequest(); @@ -722,7 +746,7 @@ describe('ThrottlingRequestManager', () => { test('a domain that has run out of work is finished, not stalled', async () => { const manager = await stallingManager(); manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - stallFor(manager, 'example.com'); + stallFor(manager, 'https://example.com/1'); expect((await manager.checkReadiness()).status).not.toBe('stalled'); }); @@ -737,7 +761,7 @@ describe('ThrottlingRequestManager', () => { await manager.addRequest({ url: 'https://example.com/1' }); // The crawl spent longer than the whole stall window elsewhere before this domain was touched. - await sleep(100); + advanceClock(100); // The first 429 starts the clock - it does not arrive with the idle time already on it. manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); @@ -759,8 +783,7 @@ describe('ThrottlingRequestManager', () => { // A single old 429, and nothing since - which is what a `Crawl-delay` longer than the stall window // looks like. The domain is not turning us away, we are keeping our distance from it. manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited' }); - stallFor(manager, 'example.com'); - domainState(manager, 'example.com').lastRateLimitedAt -= 60_000; + advanceClock(60_000); expect((await manager.checkReadiness()).status).not.toBe('stalled'); }); @@ -787,9 +810,7 @@ describe('ThrottlingRequestManager', () => { manager.recordPacingSignal({ url: 'https://example.com/1', reason: 'rateLimited', waitMs: 30_000 }), ).toBe(true); - const state = domainState(manager, 'example.com'); - expect(state.consecutive429Count).toBe(1); - expect(state.backoffUntil).toBeGreaterThan(Date.now() + 25_000); + expect(await readyAt(manager)).toBeGreaterThan(Date.now() + 25_000); // The longer of the two clocks wins, so the domain stays parked. expect(await manager.fetchNextRequest()).toBeNull(); @@ -938,7 +959,6 @@ describe('ThrottlingRequestManager', () => { expect((await manager.fetchNextRequest())!.url).toBe('https://a.example.com/1'); // A different host, but the same site - so it waits out the delay rather than doubling the rate. expect(await manager.fetchNextRequest()).toBeNull(); - expect(domainState(manager, 'example.com')).toBeDefined(); }); test('hosts with no registrable domain are still paced per hostname', async () => { @@ -1153,7 +1173,7 @@ describe('ThrottlingRequestManager', () => { await manager.fetchNextRequest(); // The dispatch armed the configured minute, not the second it was just handed. - expect(domainState(manager, 'example.com').crawlDelayUntil).toBeGreaterThan(Date.now() + 30_000); + expect(await readyAt(manager)).toBeGreaterThan(Date.now() + 30_000); }); test('a manager that paces nothing reports it as unhandled', async () => { diff --git a/packages/browser-crawler/src/internals/browser-crawler.ts b/packages/browser-crawler/src/internals/browser-crawler.ts index 60196ee81a71..a9d1b1d8a37f 100644 --- a/packages/browser-crawler/src/internals/browser-crawler.ts +++ b/packages/browser-crawler/src/internals/browser-crawler.ts @@ -660,6 +660,7 @@ export abstract class BrowserCrawler< } as unknown as Partial; } + // oxlint-disable-next-line crawlee/prefer-private-fields -- patched by @crawlee/otel private async navigate(crawlingContext: Context): Promise> { tryCancel(); diff --git a/packages/browser-pool/src/browser-pool.ts b/packages/browser-pool/src/browser-pool.ts index 9d9d0c6c3ee8..6d7ebdc61d6d 100644 --- a/packages/browser-pool/src/browser-pool.ts +++ b/packages/browser-pool/src/browser-pool.ts @@ -351,11 +351,7 @@ export class BrowserPool< fingerprintGenerator?: FingerprintGenerator; fingerprintCache?: QuickLRU; - // kept as TS-private: tests replace this interval through bracket access - private browserKillerInterval? = setInterval( - async () => this.closeInactiveRetiredBrowsers(), - BROWSER_KILLER_INTERVAL_MILLIS, - ); + private browserKillerInterval?: NodeJS.Timeout; #browserRetireInterval?: NodeJS.Timeout; @@ -366,8 +362,6 @@ export class BrowserPool< super(); this.#log = serviceLocator.getLogger().child({ prefix: 'BrowserPool' }); - this.browserKillerInterval!.unref(); - const { browserPlugins, maxOpenPagesPerBrowser, @@ -406,6 +400,14 @@ export class BrowserPool< this.retireBrowserAfterPageCount = retireBrowserAfterPageCount; this.operationTimeoutMillis = operationTimeoutSecs * 1000; this.closeInactiveBrowserAfterMillis = closeInactiveBrowserAfterSecs * 1000; + + // Sweeping slower than the window it enforces would round any sub-10s + // `closeInactiveBrowserAfterSecs` up to the sweep period. + this.browserKillerInterval = setInterval( + async () => this.closeInactiveRetiredBrowsers(), + Math.min(BROWSER_KILLER_INTERVAL_MILLIS, this.closeInactiveBrowserAfterMillis), + ); + this.browserKillerInterval.unref(); this.useFingerprints = useFingerprints; this.fingerprintOptions = fingerprintOptions; diff --git a/packages/core/test/storages/open-storage-with-different-client-should-be-respected.test.ts b/packages/core/test/storages/open-storage-with-different-client-should-be-respected.test.ts index 04c1769e5bb6..2c24c04fbde3 100644 --- a/packages/core/test/storages/open-storage-with-different-client-should-be-respected.test.ts +++ b/packages/core/test/storages/open-storage-with-different-client-should-be-respected.test.ts @@ -12,18 +12,18 @@ describe('Opening a storage with a different storage backend should be respected test('opening a RequestQueue with default client from Configuration', async () => { const queue = await RequestQueue.open({ name: 'test-rq-open-client-from-config' }); - // The sub-backend should have been created by newClient (MemoryStorageBackend), - // so its internal `storageBackend` field should reference newClient. - expect((queue.backend as any).storageBackend).toBe(newClient); + // The queue was opened without an explicit backend, so it must live in the one the service + // locator hands out - and nowhere else. + await expect(newClient.storageExists(queue.id, 'RequestQueue')).resolves.toBe(true); + await expect(new MemoryStorageBackend().storageExists(queue.id, 'RequestQueue')).resolves.toBe(false); }); test('opening a RequestQueue with a different client', async () => { const thirdClient = new MemoryStorageBackend(); - // @ts-expect-error Using this to ensure the test/impl works - thirdClient._name = 'third-client'; const queue = await RequestQueue.open({ name: 'test-rq-open-custom-client' }, { storageBackend: thirdClient }); - expect((queue.backend as any).storageBackend).toBe(thirdClient); + await expect(thirdClient.storageExists(queue.id, 'RequestQueue')).resolves.toBe(true); + await expect(newClient.storageExists(queue.id, 'RequestQueue')).resolves.toBe(false); }); }); diff --git a/packages/http-crawler/src/internals/http-crawler.ts b/packages/http-crawler/src/internals/http-crawler.ts index 9667de2413dd..73578a3eca75 100644 --- a/packages/http-crawler/src/internals/http-crawler.ts +++ b/packages/http-crawler/src/internals/http-crawler.ts @@ -523,6 +523,7 @@ export class HttpCrawler< return {}; } + // oxlint-disable-next-line crawlee/prefer-private-fields -- patched by @crawlee/otel private async makeHttpRequest( crawlingContext: CrawlingContext, ): Promise & Partial> { @@ -722,15 +723,14 @@ export class HttpCrawler< private async parseResponse(request: CrawlingRequest, response: Response) { const { status } = response; const { type, charset } = parseContentTypeFromResponse(response); - const { response: reencodedResponse, encoding } = this.encodeResponse(request, response, charset); - const contentType = { type, encoding }; if (status >= 400 && status <= 599) { this.statistics.registerStatusCode(status); } if (this.isErrorStatusCode(status)) { - const body = await reencodedResponse.text(); // TODO - this always uses UTF-8 (see https://developer.mozilla.org/en-US/docs/Web/API/Request/text) + const { response: decoded } = this.encodeResponse(request, response, charset); + const body = await decoded.text(); // TODO - this always uses UTF-8 (see https://developer.mozilla.org/en-US/docs/Web/API/Request/text) // Errors are often sent as JSON, so attempt to parse them, // despite Accept header being set to text/html. @@ -747,25 +747,32 @@ export class HttpCrawler< // It's not a JSON, so it's probably some text. Get the first 100 chars of it. throw new Error(`${status} - Internal Server Error: ${body.slice(0, 100)}`); - } else if (HTML_AND_XML_MIME_TYPES.includes(type)) { - if (!charset && !this.#forceResponseEncoding) { - const rawBytes = Buffer.from(await response.arrayBuffer()); - const metaCharset = extractCharsetFromHtmlBytes(rawBytes); - const charsetToUse = metaCharset ?? this.#suggestResponseEncoding ?? 'utf-8'; - const body = iconv.encodingExists(charsetToUse) - ? iconv.decode(rawBytes, charsetToUse) - : rawBytes.toString('utf8'); - return { response, contentType: { type, encoding: 'utf-8' as BufferEncoding }, body }; - } + } + + if (HTML_AND_XML_MIME_TYPES.includes(type) && !charset && !this.#forceResponseEncoding) { + // The charset comes from the document itself, so the raw bytes are what we need - + // decoding them through `encodeResponse` first would consume the body for nothing. + const rawBytes = Buffer.from(await response.arrayBuffer()); + const metaCharset = extractCharsetFromHtmlBytes(rawBytes); + const charsetToUse = metaCharset ?? this.#suggestResponseEncoding ?? 'utf-8'; + const body = iconv.encodingExists(charsetToUse) + ? iconv.decode(rawBytes, charsetToUse) + : rawBytes.toString('utf8'); + return { response, contentType: { type, encoding: 'utf-8' as BufferEncoding }, body }; + } + + const { response: reencodedResponse, encoding } = this.encodeResponse(request, response, charset); + const contentType = { type, encoding }; + + if (HTML_AND_XML_MIME_TYPES.includes(type)) { return { response, contentType, body: await reencodedResponse.text() }; - } else { - const body = Buffer.from(await reencodedResponse.bytes()); - return { - body, - response, - contentType, - }; } + + return { + body: Buffer.from(await reencodedResponse.bytes()), + response, + contentType, + }; } /** diff --git a/scripts/oxlint-plugin.test.ts b/scripts/oxlint-plugin.test.ts index d4e06cf1cd5c..ae60161524d7 100644 --- a/scripts/oxlint-plugin.test.ts +++ b/scripts/oxlint-plugin.test.ts @@ -19,6 +19,8 @@ ruleTester.run('prefer-private-fields', preferPrivateFields, { 'class A { public a = 1; }', 'class A { constructor(readonly a: number) {} }', 'abstract class A { protected abstract a(): void; }', + // No native equivalent, so the rule has to let it through. + 'class A { private constructor() {} }', ], invalid: [ { name: 'field', code: 'class A { private a = 1; }', errors: [{ messageId: 'preferHash' }] }, diff --git a/scripts/oxlint-plugin.ts b/scripts/oxlint-plugin.ts index e5c3a0f5a72e..be35b8ff8f6b 100644 --- a/scripts/oxlint-plugin.ts +++ b/scripts/oxlint-plugin.ts @@ -21,6 +21,10 @@ export const preferPrivateFields = { 'PropertyDefinition[accessibility="private"], MethodDefinition[accessibility="private"], TSAbstractPropertyDefinition[accessibility="private"], TSAbstractMethodDefinition[accessibility="private"], TSParameterProperty[accessibility="private"]'( node: PrivateMember, ) { + // `private constructor` has no native counterpart - it is the only way to keep a + // class buildable only through its own factories. + if ('kind' in node && node.kind === 'constructor') return; + context.report({ node: 'key' in node ? node.key : node, messageId: 'preferHash' }); }, }; diff --git a/test/browser-pool/browser-pool.test.ts b/test/browser-pool/browser-pool.test.ts index af74541fb427..6b2c777fb11c 100644 --- a/test/browser-pool/browser-pool.test.ts +++ b/test/browser-pool/browser-pool.test.ts @@ -1,4 +1,3 @@ -/* eslint-disable dot-notation -- Accessing private properties */ import http from 'node:http'; import { promisify } from 'node:util'; @@ -67,6 +66,34 @@ describe.each([ await browserPool?.destroy(); }); + /** + * Makes the browser's own `page.close()` never settle, before the pool wraps it - a browser that + * acknowledges the close and then never destroys the target. Patching the page afterwards would + * replace the pool's wrapper instead of the close it wraps. + */ + const hangEveryPageClose = () => { + const createController = plugin.createController.bind(plugin); + // The two-plugin matrix makes the controller a union, so its own `newPage` signature is the + // corresponding intersection and nothing satisfies it. Only `close` matters here. + interface AnyController { + newPage: (...args: never[]) => Promise<{ close: () => Promise }>; + } + + const patched = (() => { + const controller = createController(); + const view = controller as unknown as AnyController; + const newPage = view.newPage.bind(controller); + view.newPage = async (...args: never[]) => { + const page = await newPage(...args); + page.close = async () => new Promise(() => {}); + return page; + }; + return controller; + }) as unknown as typeof createController; + + vitest.spyOn(plugin, 'createController').mockImplementation(patched); + }; + let target: http.Server; let unprotectedProxy: ProxyChainServer; let protectedProxy: ProxyChainServer; @@ -112,7 +139,6 @@ describe.each([ expect(browserPool.startingBrowserControllers.size).toBe(0); expect(browserPool.activeBrowserControllers.size).toBe(0); expect(browserPool.retiredBrowserControllers.size).toBe(0); - expect(browserPool['browserKillerInterval']).toBeUndefined(); }); }); @@ -161,12 +187,18 @@ describe.each([ test.skip('should allow early aborting in case of outer timeout', async () => { const timeout = browserPool.operationTimeoutMillis; browserPool.operationTimeoutMillis = 500; - // @ts-expect-error mocking private method - const spy = vitest.spyOn(BrowserPool.prototype, 'executeHooks'); + + // One counter across all four launch/page-creation hook phases, standing in for the + // pool's own hook dispatch. + const hook = vitest.fn(async () => {}); + browserPool.preLaunchHooks = [hook]; + browserPool.postLaunchHooks = [hook]; + browserPool.prePageCreateHooks = [hook]; + browserPool.postPageCreateHooks = [hook]; await browserPool.newPage(); - expect(spy).toBeCalledTimes(4); - spy.mockReset(); + expect(hook).toBeCalledTimes(4); + hook.mockClear(); await expect( addTimeoutToPromise(async () => browserPool.newPage(), 10, 'opening new page timed out'), @@ -176,7 +208,7 @@ describe.each([ // thanks to `tryCancel()` calls after each await. If we did not run // inside `addTimeoutToPromise()`, this would not work and we would get // 4 calls instead of just one. - expect(spy).toBeCalledTimes(1); + expect(hook).toBeCalledTimes(1); browserPool.operationTimeoutMillis = timeout; browserPool.retireAllBrowsers(); @@ -208,13 +240,8 @@ describe.each([ }); test('should correctly override page close', async () => { - // @ts-expect-error Private function - vitest.spyOn(browserPool!, 'overridePageClose'); - const page = await browserPool.newPage(); - expect(browserPool['overridePageClose']).toBeCalled(); - const controller = browserPool.getBrowserControllerByPage(page)!; expect(controller.activePages).toEqual(1); @@ -227,6 +254,8 @@ describe.each([ }); test("should do the pool's bookkeeping for a page whose close never settles", async () => { + hangEveryPageClose(); + const page = await browserPool.newPage(); const pageId = browserPool.getPageId(page)!; const controller = browserPool.getBrowserControllerByPage(page)!; @@ -235,17 +264,12 @@ describe.each([ expect(controller.activePages).toEqual(1); - // A browser can acknowledge the close and then never destroy the target, so neither - // the promise nor the page's own `close` event ever arrives. - (page as { close: () => Promise }).close = () => new Promise(() => {}); - browserPool['overridePageClose'](page); - await page.close(); // None of this used to run: the override was parked on the un-timed close, so the // browser kept a slot it could never get back. expect(controller.activePages).toEqual(0); - expect(browserPool['pages'].has(pageId)).toBe(false); + expect(browserPool.pages.has(pageId)).toBe(false); expect(pageClosed).toHaveBeenCalled(); // The page is still attached, so the browser cannot be trusted with more work. @@ -288,7 +312,7 @@ describe.each([ expect(hookStarted).toBe(true); expect(hookFinished).toBe(false); expect(controller.activePages).toEqual(0); - expect(pool['pages'].has(pageId)).toBe(false); + expect(pool.pages.has(pageId)).toBe(false); // Retirement is keyed on the page, not on the timeout, so a slow hook must not // cost a browser that closed its page just fine. expect(pool.activeBrowserControllers.has(controller)).toBe(true); @@ -300,14 +324,13 @@ describe.each([ }, 30_000); test('should not count the page twice when its close event arrives later', async () => { + hangEveryPageClose(); + // `any` because the two-plugin matrix makes `page` a union, while the controller it // came from is a union too, so its parameter is the corresponding intersection. const page: any = await browserPool.newPage(); const controller = browserPool.getBrowserControllerByPage(page)!; - page.close = () => new Promise(() => {}); - browserPool['overridePageClose'](page); - await page.close(); expect(controller.activePages).toEqual(0); @@ -319,6 +342,8 @@ describe.each([ }); test('should run a page teardown even when the close event never arrives', async () => { + hangEveryPageClose(); + const page: any = await browserPool.newPage(); const controller = browserPool.getBrowserControllerByPage(page)!; @@ -330,9 +355,6 @@ describe.each([ tornDown++; }); - page.close = () => new Promise(() => {}); - browserPool['overridePageClose'](page); - await page.close(); await new Promise((resolve) => setImmediate(resolve)); @@ -345,31 +367,29 @@ describe.each([ }); test('should let closePage() return on a page whose close never settles', async () => { + hangEveryPageClose(); + const page = await browserPool.newPage(); const pageId = browserPool.getPageId(page)!; const controller = browserPool.getBrowserControllerByPage(page)!; - (page as { close: () => Promise }).close = () => new Promise(() => {}); - browserPool['overridePageClose'](page); - // `closePage` is the documented way a crawler returns a page, and the bound it needs // lives in the override it calls. Bounding it here as well used to time that override // out from the outside, which is what abandoned the bookkeeping below. await expect(browserPool.closePage(page)).resolves.toBeUndefined(); expect(controller.activePages).toEqual(0); - expect(browserPool['pages'].has(pageId)).toBe(false); + expect(browserPool.pages.has(pageId)).toBe(false); expect(browserPool.retiredBrowserControllers.has(controller)).toBe(true); }, 30_000); test("should not cancel the caller's task when it gives up on a close", async () => { - const page = await browserPool.newPage(); - // The bound on the close is a `Promise.race`, not `addTimeoutToPromise`: the latter // takes its AbortController from the calling frame, so firing it here would cancel // the request handler that closed the page. - (page as { close: () => Promise }).close = () => new Promise(() => {}); - browserPool['overridePageClose'](page); + hangEveryPageClose(); + + const page = await browserPool.newPage(); await expect( addTimeoutToPromise( @@ -402,8 +422,7 @@ describe.each([ test('should allow max pages per browser', async () => { browserPool.maxOpenPagesPerBrowser = 1; - // @ts-expect-error Private function - vitest.spyOn(browserPool!, 'launchBrowser'); + vitest.spyOn(plugin, 'launch'); await browserPool.newPage(); expect(browserPool.activeBrowserControllers.size).toBe(1); @@ -412,13 +431,12 @@ describe.each([ await browserPool.newPage(); expect(browserPool.activeBrowserControllers.size).toBe(3); - expect(browserPool['launchBrowser']).toBeCalledTimes(3); + expect(plugin.launch).toBeCalledTimes(3); }); test('should allow max pages per browser - no race condition', async () => { browserPool.maxOpenPagesPerBrowser = 1; - // @ts-expect-error Private function - vitest.spyOn(browserPool, 'launchBrowser'); + vitest.spyOn(plugin, 'launch'); const usePlugin = { browserPlugin: plugin, @@ -428,39 +446,30 @@ describe.each([ expect(browserPool.activeBrowserControllers.size).toBe(2); - expect(browserPool['launchBrowser']).toBeCalledTimes(2); + expect(plugin.launch).toBeCalledTimes(2); }); test('should close retired browsers', async () => { - browserPool.retireBrowserAfterPageCount = 1; - - clearInterval(browserPool['browserKillerInterval']!); - - browserPool['browserKillerInterval'] = setInterval( - async () => browserPool['closeInactiveRetiredBrowsers'](), - 100, - ); + // Own pool: the reaper sweeps once per `closeInactiveBrowserAfterSecs`, and the 2s + // default from `beforeEach` would make this test wait for it. + const pool = new BrowserPool({ browserPlugins: [plugin], closeInactiveBrowserAfterSecs: 0.1 }); + pool.retireBrowserAfterPageCount = 1; - // @ts-expect-error Private function - vitest.spyOn(browserPool!, 'closeRetiredBrowserWithNoPages'); - expect(browserPool.retiredBrowserControllers.size).toBe(0); - - const page = await browserPool.newPage(); - const controller = browserPool.getBrowserControllerByPage(page)!; - vitest.spyOn(controller, 'close'); + try { + expect(pool.retiredBrowserControllers.size).toBe(0); - expect(browserPool.retiredBrowserControllers.size).toBe(1); - await page.close(); + const page = await pool.newPage(); + const controller = pool.getBrowserControllerByPage(page)!; + vitest.spyOn(controller, 'close'); - await new Promise((resolve) => - setTimeout(() => { - resolve(); - }, 1000), - ); + expect(pool.retiredBrowserControllers.size).toBe(1); + await page.close(); - expect(browserPool['closeRetiredBrowserWithNoPages']).toHaveBeenCalled(); - expect(controller.close).toHaveBeenCalled(); - expect(browserPool.retiredBrowserControllers.size).toBe(0); + await vitest.waitFor(() => expect(pool.retiredBrowserControllers.size).toBe(0), { timeout: 10_000 }); + expect(controller.close).toHaveBeenCalled(); + } finally { + await pool.destroy(); + } }); describe('hooks', () => { @@ -471,19 +480,18 @@ describe.each([ indexArray.push(index); }; - const hooks = new Array(10); - for (let i = 0; i < hooks.length; i++) { - hooks[i] = createAsyncHookReturningIndex(i); - } + browserPool.preLaunchHooks.push( + ...Array.from({ length: 10 }, (_, i) => createAsyncHookReturningIndex(i)), + ); - await browserPool['executeHooks'](hooks); + await browserPool.newPage(); expect(indexArray).toHaveLength(10); indexArray.forEach((v, index) => expect(v).toEqual(index)); }); test('browser lifecycle works correctly', async () => { // A browser whose launch hooks have not resolved yet must survive both inactivity - // sweeps. Sub-second windows plus a fast killer interval let each sweep run several + // sweeps. Sub-second windows let each sweep run several // times during the wait, instead of sleeping past the 2s defaults from beforeEach. // The waits stay real: a live browser is launching underneath, and faking the clock // would stall the driver's own timeouts along with the pool's. @@ -492,8 +500,6 @@ describe.each([ closeInactiveBrowserAfterSecs: 0.5, retireInactiveBrowserAfterSecs: 0.5, }); - clearInterval(pool['browserKillerInterval']!); - pool['browserKillerInterval'] = setInterval(async () => pool['closeInactiveRetiredBrowsers'](), 100); let resolvePreLaunchHook: (() => void) | null = null; let resolvePostLaunchHook: (() => void) | null = null; @@ -540,21 +546,14 @@ describe.each([ describe('preLaunchHooks', () => { test('should evaluate hook before launching browser with correct args', async () => { - const myAsyncHook = async () => Promise.resolve(); + const myAsyncHook = vitest.fn(async () => {}); browserPool.preLaunchHooks.push(myAsyncHook); - // @ts-expect-error Private function - vitest.spyOn(browserPool!, 'executeHooks'); - const page = await browserPool.newPage(); const pageId = browserPool.getPageId(page)!; const { launchContext } = browserPool.getBrowserControllerByPage(page)!; - expect(browserPool['executeHooks']).toHaveBeenNthCalledWith( - 1, - browserPool.preLaunchHooks, - pageId, - launchContext, - ); + + expect(myAsyncHook).toHaveBeenCalledWith(pageId, launchContext); }); // We had a problem where if the first newPage() call, which launches @@ -584,22 +583,14 @@ describe.each([ describe('postLaunchHooks', () => { test('should evaluate hook after launching browser with correct args', async () => { - const myAsyncHook = async () => Promise.resolve(); + const myAsyncHook = vitest.fn(async () => {}); browserPool.postLaunchHooks = [myAsyncHook]; - // @ts-expect-error Private function - vitest.spyOn(browserPool, 'executeHooks'); - const page = await browserPool.newPage(); const pageId = browserPool.getPageId(page)!; const browserController = browserPool.getBrowserControllerByPage(page)!; - expect(browserPool['executeHooks']).toHaveBeenNthCalledWith( - 2, - browserPool.postLaunchHooks, - pageId, - browserController, - ); + expect(myAsyncHook).toHaveBeenCalledWith(pageId, browserController); }); // We had a problem where if the first newPage() call, which launches @@ -643,19 +634,14 @@ describe.each([ describe('prePageCreateHooks', () => { test('should evaluate hook after launching browser with correct args', async () => { - const myAsyncHook = async () => Promise.resolve(); + const myAsyncHook = vitest.fn(async () => {}); browserPool.prePageCreateHooks = [myAsyncHook]; - // @ts-expect-error Private function - vitest.spyOn(browserPool, 'executeHooks'); - const page = await browserPool.newPage(); const pageId = browserPool.getPageId(page)!; const browserController = browserPool.getBrowserControllerByPage(page)!; - expect(browserPool['executeHooks']).toHaveBeenNthCalledWith( - 3, - browserPool.prePageCreateHooks, + expect(myAsyncHook).toHaveBeenCalledWith( pageId, browserController, browserController.launchContext.useIncognitoPages ? {} : undefined, @@ -665,64 +651,40 @@ describe.each([ describe('postPageCreateHooks', () => { test('should evaluate hook after launching browser with correct args', async () => { - const myAsyncHook = async () => Promise.resolve(); + const myAsyncHook = vitest.fn(async () => {}); browserPool.postPageCreateHooks = [myAsyncHook]; - // @ts-expect-error Private function - vitest.spyOn(browserPool, 'executeHooks'); - const page = await browserPool.newPage(); const browserController = browserPool.getBrowserControllerByPage(page); - expect(browserPool['executeHooks']).toHaveBeenNthCalledWith( - 4, - browserPool.postPageCreateHooks, - page, - browserController, - ); + expect(myAsyncHook).toHaveBeenCalledWith(page, browserController); }); }); describe('prePageCloseHooks', () => { test('should evaluate hook after launching browser with correct args', async () => { - const myAsyncHook = async () => Promise.resolve(); + const myAsyncHook = vitest.fn(async () => {}); browserPool.prePageCloseHooks = [myAsyncHook]; - // @ts-expect-error Private function - vitest.spyOn(browserPool, 'executeHooks'); - const page = await browserPool.newPage(); await page.close(); const browserController = browserPool.getBrowserControllerByPage(page); - expect(browserPool['executeHooks']).toHaveBeenNthCalledWith( - 5, - browserPool.prePageCloseHooks, - page, - browserController, - ); + expect(myAsyncHook).toHaveBeenCalledWith(page, browserController); }); }); describe('postPageCloseHooks', () => { test('should evaluate hook after launching browser with correct args', async () => { - const myAsyncHook = async () => Promise.resolve(); + const myAsyncHook = vitest.fn(async () => {}); browserPool.postPageCloseHooks = [myAsyncHook]; - // @ts-expect-error Private function - vitest.spyOn(browserPool, 'executeHooks'); - const page = await browserPool.newPage(); const pageId = browserPool.getPageId(page); await page.close(); const browserController = browserPool.getBrowserControllerByPage(page); - expect(browserPool['executeHooks']).toHaveBeenNthCalledWith( - 6, - browserPool.postPageCloseHooks, - pageId, - browserController, - ); + expect(myAsyncHook).toHaveBeenCalledWith(pageId, browserController); }); }); }); diff --git a/test/core/autoscaling/autoscaled_pool.test.ts b/test/core/autoscaling/autoscaled_pool.test.ts index 8c653aa65af6..6bba1a5b1aea 100644 --- a/test/core/autoscaling/autoscaled_pool.test.ts +++ b/test/core/autoscaling/autoscaled_pool.test.ts @@ -165,97 +165,123 @@ describe('AutoscaledPool', () => { }); describe('should scale correctly', () => { - class MockSystemStatus { - okNow: boolean; - okLately: boolean; - getCurrentStatus: () => { isSystemIdle: boolean }; - getHistoricalStatus: () => { isSystemIdle: boolean }; - - constructor(okNow: boolean, okLately: boolean) { - this.okNow = okNow; - this.okLately = okLately; - this.getCurrentStatus = () => ({ isSystemIdle: this.okNow }); - this.getHistoricalStatus = () => ({ isSystemIdle: this.okLately }); - } - } - - let pool: AutoscaledPool; - let systemStatus: MockSystemStatus; - const cb = () => {}; - beforeEach(async () => { - systemStatus = new MockSystemStatus(true, true); - pool = await makePool( + const CURRENT_HISTORY_SECS = 5; + const SNAPSHOT_HISTORY_SECS = 30; + const AUTOSCALE_INTERVAL_SECS = 10; + + /** `okNow` drives the task-gating status; `okLately` the one autoscaling decisions are made on. */ + const load = { okNow: true, okLately: true }; + + /** + * The system's only load signal, so its verdict *is* the system status. The two status windows are + * configured far apart, so the requested sample window identifies which of the two is asking. + */ + const mockLoadSignal: LoadSignal = { + name: 'mock', + overloadedRatio: 0.5, + async start() {}, + async stop() {}, + getSample: (sampleDurationMillis) => [ { - runTaskFunction: async () => {}, - isFinishedFunction: async () => false, - isTaskReadyFunction: async () => true, + createdAt: new Date(), + isOverloaded: sampleDurationMillis === CURRENT_HISTORY_SECS * 1000 ? !load.okNow : !load.okLately, }, - { minConcurrency: 1, maxConcurrency: 100 }, - ); - // Autoscaling now lives on the shared governor; mock its system status. - // @ts-expect-error Mock - pool.system.systemStatus = systemStatus; + ], + }; + + const concurrencyOptions: ConcurrencySystemOptions = { + minConcurrency: 1, + maxConcurrency: 100, + desiredConcurrencyRatio: 0.9, + scaleUpStepRatio: 0.05, + scaleDownStepRatio: 0.05, + currentHistorySecs: CURRENT_HISTORY_SECS, + snapshotHistorySecs: SNAPSHOT_HISTORY_SECS, + autoscaleIntervalSecs: AUTOSCALE_INTERVAL_SECS, + loadSignals: { + memory: false, + eventLoop: false, + cpu: false, + storageBackend: false, + custom: [mockLoadSignal], + }, + }; + + const taskOptions = { + runTaskFunction: async () => {}, + isFinishedFunction: async () => false, + isTaskReadyFunction: async () => true, + }; + + beforeEach(() => { + load.okNow = true; + load.okLately = true; }); - test('works with low values', () => { - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); + /** + * Autoscaling is driven by an interval that `ConcurrencySystem.start()` sets up, so the loop is stepped by + * advancing that interval. Only the timer functions the interval itself uses are faked, leaving sleeps and + * the snapshotter's teardown on real ones. + */ + function useAutoscaleTimer() { + vitest.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); + onTestFinished(() => { + vitest.useRealTimers(); + }); + } + + const tick = async () => vitest.advanceTimersByTimeAsync(AUTOSCALE_INTERVAL_SECS * 1000); + + test('works with low values', async () => { + useAutoscaleTimer(); + const pool = await makePool(taskOptions, concurrencyOptions); + // The loop also runs once during `start()`, so scaling is measured from a known value rather than from + // whatever that first turn left behind. + systemOf(pool).desiredConcurrency = 1; + + await tick(); expect(pool.desiredConcurrency).toBe(2); - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); - expect(pool.desiredConcurrency).toBe(2); // because currentConcurrency is not high enough; + await tick(); + expect(pool.desiredConcurrency).toBe(2); // because currentConcurrency is not high enough systemOf(pool).tryRegisterTaskStart(); systemOf(pool).tryRegisterTaskStart(); - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); + await tick(); expect(pool.desiredConcurrency).toBe(3); - systemStatus.okNow = false; // this should have no effect + load.okNow = false; // this should have no effect systemOf(pool).tryRegisterTaskStart(); - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); + await tick(); expect(pool.desiredConcurrency).toBe(4); - systemStatus.okLately = false; - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); + load.okLately = false; + await tick(); expect(pool.desiredConcurrency).toBe(3); }); - test('works with high values', () => { - // Should not scale because current concurrency is too low. + test('works with high values', async () => { + useAutoscaleTimer(); + const pool = await makePool(taskOptions, concurrencyOptions); systemOf(pool).desiredConcurrency = 50; - const targetConcurrency = Math.floor( - // @ts-expect-error Accessing private prop on the governor - pool.desiredConcurrency * pool.system.desiredConcurrencyRatio, - ); - for (let i = 0; i < targetConcurrency - 1; i++) { + + // Should not scale because current concurrency is too low - one short of the 90% of 50 that scaling up + // asks for. + for (let i = 0; i < 44; i++) { systemOf(pool).tryRegisterTaskStart(); } - systemStatus.okLately = true; - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); + await tick(); expect(pool.desiredConcurrency).toBe(50); - // Should scale because we bumped up current concurrency. + // Should scale because we bumped up current concurrency, by ceil(50 * 0.05). systemOf(pool).tryRegisterTaskStart(); - let newConcurrency = - // @ts-expect-error Accessing private prop on the governor - pool.desiredConcurrency + Math.ceil(pool.desiredConcurrency * pool.system.scaleUpStepRatio); - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); - expect(pool.desiredConcurrency).toEqual(newConcurrency); - - // Should scale down. - systemStatus.okLately = false; - newConcurrency = - // @ts-expect-error Accessing private prop on the governor - pool.desiredConcurrency - Math.ceil(pool.desiredConcurrency * pool.system.scaleDownStepRatio); - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(cb); - expect(pool.desiredConcurrency).toEqual(newConcurrency); + await tick(); + expect(pool.desiredConcurrency).toBe(53); + + // Should scale down, by ceil(53 * 0.05). + load.okLately = false; + await tick(); + expect(pool.desiredConcurrency).toBe(50); }); test('works at minConcurrency when currently overloaded', async () => { @@ -263,8 +289,7 @@ describe('AutoscaledPool', () => { let concurrencyLog: number[] = []; let count = 0; - // The task loop is configuration, so this test builds its own pool instead of reusing the shared one. - pool = await makePool( + const pool = await makePool( { runTaskFunction: async () => { await sleep(10); @@ -273,11 +298,9 @@ describe('AutoscaledPool', () => { isFinishedFunction: async () => count >= limit, isTaskReadyFunction: async () => count < limit, }, - { minConcurrency: 1, maxConcurrency: 100 }, + concurrencyOptions, ); - // @ts-expect-error Mock - pool.system.systemStatus = systemStatus; - systemStatus.okNow = false; + load.okNow = false; systemOf(pool).desiredConcurrency = 10; const origStart = pool.system.tryRegisterTaskStart.bind(pool.system); @@ -560,6 +583,8 @@ describe('AutoscaledPool', () => { }); test('should work with loggingIntervalSecs = null', async () => { + // The autoscaling loop turns once during `start()`; a throw there surfaces as an unhandled rejection, + // which fails the run rather than this test. const pool = await makePool( { runTaskFunction: async () => Promise.resolve(), @@ -568,8 +593,6 @@ describe('AutoscaledPool', () => { }, { minConcurrency: 1, maxConcurrency: 100, loggingIntervalSecs: null }, ); - // @ts-expect-error Calling private method on the governor - pool.system.autoscale(() => {}); expect(pool.desiredConcurrency).toBe(2); }); diff --git a/test/core/autoscaling/concurrency_system.test.ts b/test/core/autoscaling/concurrency_system.test.ts index f2ffacef3da6..6cf2a1982781 100644 --- a/test/core/autoscaling/concurrency_system.test.ts +++ b/test/core/autoscaling/concurrency_system.test.ts @@ -1,9 +1,22 @@ import type { ConcurrencyConsumer, IConcurrencySystem, LoadSignal, LoadSignalStartContext } from '@crawlee/basic'; import { AutoscaledPool, ConcurrencySystem, EventLoopLoadSignal, SnapshotStore } from '@crawlee/basic'; +import { serviceLocator } from '@crawlee/core'; import { sleep } from '@crawlee/utils'; import log from '@apify/log'; +/** + * A custom load signal with observable lifecycle hooks. Custom signals are started and stopped on exactly the same + * path as the built-in ones, so this is how a system's lifecycle is watched from the outside. + */ +const spySignal = (name = 'spySignal') => ({ + name, + overloadedRatio: 0.5, + start: vitest.fn(async (_context: LoadSignalStartContext) => {}), + stop: vitest.fn(async () => {}), + getSample: () => [], +}); + describe('ConcurrencySystem', () => { let logLevel: number; beforeAll(() => { @@ -221,12 +234,19 @@ describe('ConcurrencySystem', () => { loadSignals: { memory: false, eventLoop: false, cpu: false, storageBackend: false }, }); - // @ts-expect-error Accessing private prop - expect(system.snapshotter.getLoadSignals()).toEqual([]); - await system.start(); try { - expect(system.getCurrentStatus().isSystemIdle).toBe(true); + // Nothing is left to report on, so every built-in field is the disabled placeholder rather than a + // real verdict - a signal still running would report its own `overloadedRatio` as `limitRatio`. + const idle = { isOverloaded: false, limitRatio: 0, actualRatio: 0 }; + const status = system.getCurrentStatus(); + expect(status.memInfo).toEqual(idle); + expect(status.eventLoopInfo).toEqual(idle); + expect(status.cpuInfo).toEqual(idle); + expect(status.storageBackendInfo).toEqual(idle); + expect(status.loadSignalInfo).toBeUndefined(); + + expect(status.isSystemIdle).toBe(true); expect(system.hasCapacityForTask()).toBe(true); } finally { await system.stop(); @@ -297,29 +317,25 @@ describe('ConcurrencySystem', () => { describe('lifecycle', () => { test('start()/stop() are idempotent', async () => { - const system = new ConcurrencySystem(); - // @ts-expect-error Accessing private prop - const snapshotter = system.snapshotter; - const startSpy = vitest.spyOn(snapshotter, 'start'); - const stopSpy = vitest.spyOn(snapshotter, 'stop'); + const signal = spySignal(); + const system = new ConcurrencySystem({ loadSignals: { custom: [signal] } }); // A second start() on an already-running system is a no-op... await system.start(); await system.start(); - expect(startSpy).toHaveBeenCalledTimes(1); + expect(signal.start).toHaveBeenCalledTimes(1); // ...and so is a second stop(). await system.stop(); await system.stop(); - expect(stopSpy).toHaveBeenCalledTimes(1); + expect(signal.stop).toHaveBeenCalledTimes(1); }); test('stop() is a no-op when never started', async () => { - const system = new ConcurrencySystem(); - // @ts-expect-error Accessing private prop - const stopSpy = vitest.spyOn(system.snapshotter, 'stop'); + const signal = spySignal(); + const system = new ConcurrencySystem({ loadSignals: { custom: [signal] } }); await system.stop(); - expect(stopSpy).not.toHaveBeenCalled(); + expect(signal.stop).not.toHaveBeenCalled(); }); test('isRunning reflects the lifecycle', async () => { @@ -382,15 +398,14 @@ describe('ConcurrencySystem', () => { getSample: () => [], }; - const system = new ConcurrencySystem({ loadSignals: { custom: [signal] } }); - // @ts-expect-error Accessing private prop - const snapshotterStop = vitest.spyOn(system.snapshotter, 'stop'); + const healthy = spySignal('healthySignal'); + const system = new ConcurrencySystem({ loadSignals: { custom: [signal, healthy] } }); await expect(system.start()).rejects.toThrow('signal boot failed'); - // The built-in signals started before the custom one blew up, so the failed attempt has to unwind them - // instead of leaving their intervals behind - and must not report a system that is up. - expect(snapshotterStop).toHaveBeenCalledTimes(1); + // The signals that did start before the other one blew up have to be unwound instead of leaving their + // collection running - and the failed attempt must not report a system that is up. + expect(healthy.stop).toHaveBeenCalledTimes(1); expect(system.isRunning).toBe(false); // A retry has to actually retry, rather than resolving instantly against the memoized failure. @@ -402,15 +417,14 @@ describe('ConcurrencySystem', () => { }); test('start() after stop() restarts the system', async () => { - const system = new ConcurrencySystem(); - // @ts-expect-error Accessing private prop - const snapshotterStart = vitest.spyOn(system.snapshotter, 'start'); + const signal = spySignal(); + const system = new ConcurrencySystem({ loadSignals: { custom: [signal] } }); await system.start(); await system.stop(); await system.start(); - expect(snapshotterStart).toHaveBeenCalledTimes(2); + expect(signal.start).toHaveBeenCalledTimes(2); expect(system.isRunning).toBe(true); await system.stop(); @@ -438,9 +452,13 @@ describe('ConcurrencySystem', () => { }); test('capacity queried on a stopped system warns once per session', async () => { + // The system logs through a child of the registered logger; collapsing `child()` onto its parent lets + // the spy below observe it. + const logger = serviceLocator.getLogger(); + vitest.spyOn(logger, 'child').mockReturnValue(logger); + const warning = vitest.spyOn(logger, 'warning').mockImplementation(() => {}); + const system = new ConcurrencySystem(); - // @ts-expect-error Accessing private prop - const warning = vitest.spyOn(system.log, 'warning'); await system.start(); system.hasCapacityForTask(); diff --git a/test/core/autoscaling/snapshotter.test.ts b/test/core/autoscaling/snapshotter.test.ts index 1c47c1a2bcd5..9a43bb0399ba 100644 --- a/test/core/autoscaling/snapshotter.test.ts +++ b/test/core/autoscaling/snapshotter.test.ts @@ -392,7 +392,7 @@ describe('Snapshotter', () => { }; // Mock memory info to be able to inject custom memory measurement data. - vitest.spyOn(LocalEventManager.prototype as any, 'getMemoryInfo').mockResolvedValue(memoryData); + vitest.spyOn(utils, 'getMemoryInfo').mockResolvedValue(memoryData); let configuration: Configuration; if (dynamic) { diff --git a/test/core/browser_launchers/playwright_launcher.test.ts b/test/core/browser_launchers/playwright_launcher.test.ts index 374edac79e67..2f08dac748ad 100644 --- a/test/core/browser_launchers/playwright_launcher.test.ts +++ b/test/core/browser_launchers/playwright_launcher.test.ts @@ -5,13 +5,7 @@ import type { AddressInfo } from 'node:net'; import path from 'node:path'; import util from 'node:util'; -import { - BrowserLauncher, - Configuration, - launchPlaywright, - PlaywrightLauncher, - serviceLocator, -} from '@crawlee/playwright'; +import { Configuration, launchPlaywright, PlaywrightLauncher, serviceLocator } from '@crawlee/playwright'; // @ts-expect-error no types import basicAuthParser from 'basic-auth-parser'; import type { Browser, BrowserType } from 'playwright'; @@ -147,7 +141,6 @@ describe('launchPlaywright()', () => { }); test('supports useChrome option', async () => { - const spy = vitest.spyOn(BrowserLauncher.prototype as any, 'getTypicalChromeExecutablePath'); let browser; const opts = { useChrome: true, @@ -166,7 +159,6 @@ describe('launchPlaywright()', () => { expect(title).toBe('Example Domain'); expect(version).not.toMatch('Chromium'); - expect(spy).toBeCalledTimes(1); } finally { if (browser) await browser.close(); } @@ -193,14 +185,19 @@ describe('launchPlaywright()', () => { }); test('does not use default when using chrome', () => { - const launcher = new PlaywrightLauncher({ - useChrome: true, - launcher: {} as BrowserType, - }); + const chromeExecutablePath = 'chromeExecutablePath'; + const launcher = new PlaywrightLauncher( + { + useChrome: true, + launcher: {} as BrowserType, + }, + // `CRAWLEE_DEFAULT_BROWSER_PATH` is still set by this describe's `beforeAll`; the Chrome + // path is configured explicitly so the expected value doesn't depend on the current OS. + new Configuration({ chromeExecutablePath }), + ); const plugin = launcher.createBrowserPlugin(); - // @ts-expect-error private method - expect(plugin.launchOptions.executablePath).toBe(launcher.getTypicalChromeExecutablePath()); + expect(plugin.launchOptions!.executablePath).toBe(chromeExecutablePath); }, 60e3); test('allows to be overridden', () => { diff --git a/test/core/browser_launchers/puppeteer_launcher.test.ts b/test/core/browser_launchers/puppeteer_launcher.test.ts index 6036f6ac0a32..15020c406c73 100644 --- a/test/core/browser_launchers/puppeteer_launcher.test.ts +++ b/test/core/browser_launchers/puppeteer_launcher.test.ts @@ -5,7 +5,7 @@ import type { AddressInfo } from 'node:net'; import path from 'node:path'; import util from 'node:util'; -import { BrowserLauncher, launchPuppeteer } from '@crawlee/puppeteer'; +import { launchPuppeteer } from '@crawlee/puppeteer'; import type { Dictionary } from '@crawlee/types'; // @ts-expect-error no types import basicAuthParser from 'basic-auth-parser'; @@ -199,8 +199,6 @@ describe('launchPuppeteer()', () => { }); test('supports useChrome option', async () => { - const spy = vitest.spyOn(BrowserLauncher.prototype as any, 'getTypicalChromeExecutablePath'); - let browser; const opts = { useChrome: true, @@ -222,7 +220,6 @@ describe('launchPuppeteer()', () => { expect(title).toBe('Example Domain'); expect(version).toMatch('Chrome'); expect(version).not.toMatch('Chromium'); - expect(spy).toBeCalledTimes(1); } finally { if (browser) await browser.close(); } diff --git a/test/core/crawlers/adaptive_playwright_crawler.test.ts b/test/core/crawlers/adaptive_playwright_crawler.test.ts index 0ecea9199ffe..4d2cf0361af2 100644 --- a/test/core/crawlers/adaptive_playwright_crawler.test.ts +++ b/test/core/crawlers/adaptive_playwright_crawler.test.ts @@ -22,7 +22,6 @@ import type { import { AdaptivePlaywrightCrawler, adaptivePlaywrightCrawlerStatisticState, - BasicCrawler, createAdaptivePlaywrightRouter, fullResultComparator, playwrightBrowserPool, @@ -41,16 +40,19 @@ import { startExpressAppPromise } from '../../shared/_helper.js'; // A minimal logger that records every message into a shared array. Child loggers share the same // array, so messages emitted by the crawler's prefixed child logger are captured as well. class RecordingLogger extends BaseCrawleeLogger { - constructor(private readonly messages: string[]) { + readonly #messages: string[]; + + constructor(messages: string[]) { super(); + this.#messages = messages; } logWithLevel(_level: number, message: string): void { - this.messages.push(message); + this.#messages.push(message); } protected createChild(_options: Partial): CrawleeLogger { - return new RecordingLogger(this.messages); + return new RecordingLogger(this.#messages); } } @@ -142,13 +144,6 @@ describe('AdaptivePlaywrightCrawler', () => { // each test, which clears the storage-instance cache; here we just install a fresh in-memory // storage backend for this suite. serviceLocator.setStorageBackend(new MemoryStorageBackend()); - // `BasicCrawler` keeps a process-global instance counter that assigns each crawler a distinct - // default request queue (the first one uses the shared default queue, later ones get their own - // `__default___` alias). Since every test wipes storage and starts fresh, the counter must be - // reset too — otherwise later crawlers open aliased queues that are out of sync with the freshly - // reset storage, and the crawler restores a stale handled-request count and processes nothing. - // @ts-expect-error Reset private static instance counter for test isolation - BasicCrawler.instanceCount = 0; lastDynamicRequestUserAgent = undefined; }); diff --git a/test/core/crawlers/basic_crawler.test.ts b/test/core/crawlers/basic_crawler.test.ts index 59dd56c3ae42..2b0d51a8ba91 100644 --- a/test/core/crawlers/basic_crawler.test.ts +++ b/test/core/crawlers/basic_crawler.test.ts @@ -42,7 +42,8 @@ import { Statistics, ThrottlingRequestManager, } from '@crawlee/basic'; -import { MemoryStorageBackend } from '@crawlee/core'; +import type { StorageTransaction } from '@crawlee/core'; +import { currentStorageTransaction, MemoryStorageBackend } from '@crawlee/core'; import { BaseHttpClient } from '@crawlee/http-client'; import type { Dictionary, ISession, ProxyInfo } from '@crawlee/types'; import { RobotsTxtFile, sleep } from '@crawlee/utils'; @@ -825,13 +826,15 @@ describe('BasicCrawler', () => { test('drains foreground and background robots.txt skips exactly once', async () => { const onSkippedRequest = vitest.fn(); - const crawler = new BasicCrawler({ + const crawler = new (class MockedRobotsTxtCrawler extends BasicCrawler { + override async getRobotsTxtFileForUrl(_: string) { + return RobotsTxtFile.from('https://example.com/robots.txt', 'User-agent: *\nDisallow: /denied\n'); + } + })({ + respectRobotsTxtFile: true, requestHandler: async () => {}, onSkippedRequest, }); - vitest - .spyOn(crawler as any, 'isAllowedBasedOnRobotsTxtFile') - .mockImplementation(async (url: unknown) => !String(url).includes('/denied')); const warningSpy = vitest.spyOn(crawler.log, 'warning'); const foregroundDenied = 'https://example.com/denied/foreground'; @@ -896,8 +899,7 @@ describe('BasicCrawler', () => { minConcurrency: (crawler.concurrencySystem! as ConcurrencySystem).minConcurrency, maxConcurrency: (crawler.concurrencySystem! as ConcurrencySystem).maxConcurrency, desiredConcurrency: (crawler.concurrencySystem! as ConcurrencySystem).desiredConcurrency, - // eslint-disable-next-line dot-notation -- private member on the governor - maxTasksPerMinute: (crawler.concurrencySystem! as ConcurrencySystem)['maxTasksPerMinute'], + maxTasksPerMinute: (crawler.concurrencySystem! as ConcurrencySystem).maxTasksPerMinute, }); // Shortcuts feed the default ConcurrencySystem the crawler builds. @@ -1562,17 +1564,31 @@ describe('BasicCrawler', () => { vitest.restoreAllMocks(); }); - test('should say that task is not ready requestList is not set and requestQueue is empty', async () => { + test('does not start a task while the request manager is not ready', async () => { const requestQueue = await RequestQueue.open({ id: 'xxx' }); - requestQueue.checkReadiness = async () => Promise.resolve({ status: 'waiting' }); + await requestQueue.addRequest({ url: 'https://example.com' }); + + // The queue reports readiness only once we let it - until then the crawler must not fetch anything. + const checkReadiness = requestQueue.checkReadiness.bind(requestQueue); + let ready = false; + requestQueue.checkReadiness = async () => (ready ? checkReadiness() : { status: 'waiting' }); + const handled: string[] = []; const crawler = new BasicCrawler({ requestQueue, - requestHandler: async () => {}, + requestHandler: async ({ request }) => { + handled.push(request.url); + }, + taskLoopOptions: { maybeRunIntervalSecs: 0.05 }, }); - // @ts-expect-error Accessing private prop - expect(await crawler.isTaskReadyFunction()).toBe(false); + const run = crawler.run(); + await sleep(300); + expect(handled).toEqual([]); + + ready = true; + await run; + expect(handled).toEqual(['https://example.com']); }); test('should be possible to override isFinishedFunction and isTaskReadyFunction via taskLoopOptions', async () => { @@ -2071,21 +2087,29 @@ describe('BasicCrawler', () => { const url = 'https://example.com'; const requestList = await RequestList.open({ sources: [{ url }] }); - const results = []; + const handled: string[] = []; + const failures: string[] = []; const crawler = new BasicCrawler({ requestList, requestHandlerTimeoutSecs: Infinity, - maxRequestRetries: 1, - requestHandler: async () => sleep(1000), - failedRequestHandler: async ({ request }) => { - results.push(request); + maxRequestRetries: 0, + requestHandler: async ({ request }) => { + // An unclamped `Infinity` overflows `setTimeout`, which then fires on the next tick and + // kills the handler immediately instead of never (see #4043). + await sleep(50); + handled.push(request.url); + }, + failedRequestHandler: async (_context, error) => { + failures.push(error.message); }, }); - const maxSignedInteger = 2 ** 31 - 1; - expect(crawler['resolveRequestHandlerTimeoutMillis'](undefined)).toBe(maxSignedInteger); - // @ts-expect-error Accessing private prop - expect(crawler.internalTimeoutMillis).toBe(maxSignedInteger); + await crawler.run(); + + expect(handled).toEqual([url]); + expect(failures).toEqual([]); + // @ts-expect-error Accessing protected prop + expect(crawler.internalTimeoutMillis).toBe(2 ** 31 - 1); }); test('should not log stack trace for timeout errors by default', async () => { @@ -2786,7 +2810,11 @@ describe('BasicCrawler', () => { const requestQueue = await RequestQueue.open(); const addRequestsBatchedSpy = vitest.spyOn(requestQueue, 'addRequestsBatched'); - const crawler = new BasicCrawler({ + const crawler = new (class MockedRobotsTxtCrawler extends BasicCrawler { + override async getRobotsTxtFileForUrl(_: string) { + return RobotsTxtFile.from('http://example.com/robots.txt', 'User-agent: *\nDisallow: /2\n'); + } + })({ requestQueue, maxRequestsPerCrawl: 2, respectRobotsTxtFile: true, @@ -2795,11 +2823,6 @@ describe('BasicCrawler', () => { crawler.statistics.state.requestsSucceeded = 0; - // Mock robots.txt checking to disallow some URLs - vitest.spyOn(crawler as any, 'isAllowedBasedOnRobotsTxtFile').mockImplementation(async (url) => { - return url !== 'http://example.com/2'; - }); - await crawler.addRequests([ 'http://example.com/1', // Allowed by robots.txt 'http://example.com/2', // Blocked by robots.txt @@ -2895,17 +2918,9 @@ describe('BasicCrawler', () => { expect(warning).not.toHaveBeenCalled(); - // The robots.txt `Crawl-delay: 5` must have reached the manager, not merely been survivable. + // The robots.txt `Crawl-delay: 5` must have reached the manager, not merely been survivable: + // after the first fetch it paces dispatch, holding the next request back instead of serving it. await requestManager.fetchNextRequest(); - // `domainStates` is TS-private, and deliberately not `#private`, so that tests can read it. - const { domainStates } = requestManager as unknown as { - domainStates: Map; - }; - const state = domainStates.get('example.com')!; - expect(state.declaredCrawlDelayMs).toBe(5_000); - - // ...and it paces dispatch: the next request is held back rather than served immediately. - expect(state.crawlDelayUntil).toBeGreaterThan(Date.now() + 4_000); expect(await requestManager.fetchNextRequest()).toBeNull(); const readiness = await requestManager.checkReadiness(); @@ -3780,17 +3795,35 @@ describe('BasicCrawler', () => { }); test('an unclosed transaction on a normal pipeline return is discarded and logged', async () => { - const crawler = new BasicCrawler({ requestHandler: async () => {} }); - const errorSpy = vitest.spyOn((crawler as any).log, 'error').mockImplementation(() => {}); - + let leaked: StorageTransaction | undefined; // Simulate the wiring bug the guard exists for: the pipeline callback returns normally while - // its transaction is still open (handleRequest failed to commit or roll it back). - let leaked: any; - await (crawler as any).runInStorageTransaction(async () => { - leaked = (await import('@crawlee/core')).currentStorageTransaction(); + // its transaction is still open (handleRequest failed to commit or roll it back). Injected + // through the public `basicContextPipeline` seam, so the crawler still runs for real. + const crawler = new (class LeakyPipelineCrawler extends BasicCrawler { + // A pipeline that returns without ever invoking the crawler's action; `ContextPipeline` + // has no way to express that, hence the cast at this one seam. + override get basicContextPipeline() { + return { + chain: () => ({ + call: async () => { + leaked = currentStorageTransaction(); + }, + }), + } as unknown as BasicCrawler['basicContextPipeline']; + } + })({ + requestHandler: async () => {}, + // Nothing ever marks the request as handled, so end the run after that single task. + taskLoopOptions: { + isTaskReadyFunction: async () => leaked === undefined, + isFinishedFunction: async () => leaked !== undefined, + }, }); + const errorSpy = vitest.spyOn(crawler.log, 'error').mockImplementation(() => {}); + + await crawler.run([`http://${HOSTNAME}:${port}/`]); - expect(leaked.state).toBe('rolledBack'); // discarded, not left open + expect(leaked!.state).toBe('rolledBack'); // discarded, not left open expect(errorSpy).toHaveBeenCalledWith(expect.stringMatching(/still open after the request pipeline/)); errorSpy.mockRestore(); @@ -3842,7 +3875,14 @@ describe('BasicCrawler', () => { }); test('onSkippedRequest bookkeeping survives an in-pipeline robots.txt skip', async () => { - const crawler = new BasicCrawler({ + const crawler = new (class MockedRobotsTxtCrawler extends BasicCrawler { + override async getRobotsTxtFileForUrl(_: string) { + return RobotsTxtFile.from( + `http://${HOSTNAME}:${port}/robots.txt`, + 'User-agent: *\nDisallow: /disallowed\n', + ); + } + })({ maxRequestRetries: 0, respectRobotsTxtFile: true, requestHandler: async () => {}, @@ -3851,11 +3891,10 @@ describe('BasicCrawler', () => { }, }); - // Let the request into the crawler's queue, then disallow it during processing, so the skip - // happens inside the request's transaction scope. + // Let the request into the crawler's queue - a direct queue add skips the robots.txt check - + // so the skip happens during processing, inside the request's transaction scope. const queue = await crawler.getRequestQueue(); await queue.addRequest({ url: `http://${HOSTNAME}:${port}/disallowed` }); - vitest.spyOn(crawler as any, 'isAllowedBasedOnRobotsTxtFile').mockResolvedValue(false); await crawler.run(); diff --git a/test/core/crawlers/browser_crawler.test.ts b/test/core/crawlers/browser_crawler.test.ts index 9234229856a0..b633aff26302 100644 --- a/test/core/crawlers/browser_crawler.test.ts +++ b/test/core/crawlers/browser_crawler.test.ts @@ -132,11 +132,8 @@ describe('BrowserCrawler', () => { expect(releaseSpy).toHaveBeenCalled(); expect(destroySpy).not.toHaveBeenCalled(); - // What a destroyed pool loses for good, since nothing re-arms either: its listeners, and the timers that - // retire idle browsers and reap the retired ones. + // What a destroyed pool loses for good, since nothing re-arms it: its listeners. expect(ownedPool.listenerCount(BROWSER_POOL_EVENTS.BROWSER_LAUNCHED)).toBe(1); - // eslint-disable-next-line dot-notation -- TS-private on the pool - expect(ownedPool['browserKillerInterval']).toBeDefined(); await browserCrawler.destroy(); expect(destroySpy).toHaveBeenCalledTimes(1); diff --git a/test/core/crawlers/cheerio_crawler.test.ts b/test/core/crawlers/cheerio_crawler.test.ts index 4c3ab0606777..903dabe54593 100644 --- a/test/core/crawlers/cheerio_crawler.test.ts +++ b/test/core/crawlers/cheerio_crawler.test.ts @@ -1,4 +1,5 @@ import { createServer, type Server } from 'node:http'; +import type { AddressInfo } from 'node:net'; import type { BasicCrawlingContext, CheerioCrawlingContext, CheerioRequestHandler, Source } from '@crawlee/cheerio'; import { FetchHttpClient } from '@crawlee/http-client'; @@ -336,8 +337,20 @@ describe('CheerioCrawler', () => { processed.push(request); }; + // a client that always outlives the navigation window; carries the BaseHttpClient prototype so + // the crawler's `z.instanceof` validation accepts it + const slowClient = Object.assign(Object.create(BaseHttpClient.prototype) as BaseHttpClient, { + sendRequest: async () => { + await sleep(300); + return new Response('Body', { + headers: { 'content-type': 'text/html' }, + }); + }, + }); + const cheerioCrawler = new CheerioCrawler({ requestList, + httpClient: slowClient, navigationTimeoutSecs: 5 / 1000, maxRequestRetries: 1, minConcurrency: 2, @@ -348,12 +361,6 @@ describe('CheerioCrawler', () => { }, }); - // @ts-expect-error Overriding private method - cheerioCrawler.requestFunction = async () => { - await sleep(300); - return 'Body'; - }; - await cheerioCrawler.run(); expect(processed).toHaveLength(0); @@ -862,47 +869,59 @@ describe('CheerioCrawler', () => { describe('should use response encoding', () => { const html = 'Žluťoučký kůň'; + // serves the very same windows-1250 bytes, with the charset in the content-type header only when asked + let encodingServer: Server; + let encodingServerUrl: string; + + beforeAll(async () => { + encodingServer = createServer((req, res) => { + const charset = new URL(req.url!, 'http://localhost').searchParams.get('charset'); + res.writeHead(200, { 'content-type': `text/html${charset ? `; charset=${charset}` : ''}` }); + res.end(iconv.encode(html, 'windows-1250')); + }); + await new Promise((resolve) => encodingServer.listen(0, resolve)); + const { port: encodingPort } = encodingServer.address() as AddressInfo; + encodingServerUrl = `http://localhost:${encodingPort}`; + }); + + afterAll(() => { + encodingServer.close(); + }); test('as a fallback', async () => { - const requestList = await RequestList.open({ - sources: ['http://useless.x'], - }); - const suggestResponseEncoding = 'windows-1250'; - const buf = iconv.encode(html, suggestResponseEncoding); // Ensure it's really encoded. - expect(buf.toString('utf8')).not.toBe(html); + expect(iconv.encode(html, 'windows-1250').toString('utf8')).not.toBe(html); + let context: CheerioCrawlingContext | null = null; const crawler = new CheerioCrawler({ - requestList, - requestHandler: () => {}, - suggestResponseEncoding, + requestHandler: (ctx) => { + context = ctx; + }, + suggestResponseEncoding: 'windows-1250', }); - // @ts-expect-error Using private method - const { response, encoding } = crawler.encodeResponse({}, new Response(new Uint8Array(buf))); - expect(encoding).toBe('utf8'); - expect(await response.text()).toBe(html); + await crawler.run([encodingServerUrl]); + + context = context as unknown as CheerioCrawlingContext; + expect(context.body).toBe(html); + expect(context.$('html').text()).toBe('Žluťoučký kůň'); }); test('always when forced', async () => { - const requestList = await RequestList.open({ - sources: ['http://useless.x'], - }); - const forceResponseEncoding = 'win1250'; - const buf = iconv.encode(html, forceResponseEncoding); - // Ensure it's really encoded. - expect(buf.toString('utf8')).not.toBe(html); - + let context: CheerioCrawlingContext | null = null; const crawler = new CheerioCrawler({ - requestList, - requestHandler: () => {}, - forceResponseEncoding, + requestHandler: (ctx) => { + context = ctx; + }, + forceResponseEncoding: 'win1250', }); - // @ts-expect-error Using private method - const { response, encoding } = crawler.encodeResponse({}, new Response(new Uint8Array(buf)), 'ascii'); - expect(encoding).toBe('utf8'); - expect(await response.text()).toBe(html); + // the header says ascii, the bytes are windows-1250 - the forced encoding must win + await crawler.run([`${encodingServerUrl}/?charset=ascii`]); + + context = context as unknown as CheerioCrawlingContext; + expect(context.contentType.encoding).toBe('utf8'); + expect(context.body).toBe(html); }); test('via http-equiv meta tag when no charset in HTTP header', async () => { diff --git a/test/core/crawlers/statistics.test.ts b/test/core/crawlers/statistics.test.ts index a0a613066641..67fe40eebb5c 100644 --- a/test/core/crawlers/statistics.test.ts +++ b/test/core/crawlers/statistics.test.ts @@ -108,9 +108,13 @@ describe('Statistics', () => { const record = (await store.getValue(persistStateKey(stats)))!; await store.setValue(persistStateKey(stats), { ...record, requestsSucceeded: 'plenty' }); + // The statistics log through a child of the registered logger; collapsing `child()` onto its + // parent lets the spy below observe it. + const logger = serviceLocator.getLogger(); + vitest.spyOn(logger, 'child').mockReturnValue(logger); + const warningSpy = vitest.spyOn(logger, 'warning').mockImplementation(() => {}); + const restored = new Statistics({ id: stats.id }); - // @ts-expect-error Accessing private prop - const warningSpy = vitest.spyOn(restored.log, 'warning').mockImplementation(() => {}); // A corrupt counter must not take the crawl down with it, and must not be trusted either - an // increment on a string would poison every later one. @@ -353,12 +357,16 @@ describe('Statistics', () => { test('should regularly log stats', async () => { const logged: [string, Dictionary | undefined | null][] = []; - // @ts-expect-error Accessing private prop - const infoSpy = vitest.spyOn(stats.log, 'info'); - infoSpy.mockImplementation((message: string, data?: Record | null) => { + // The statistics log through a child of the registered logger; collapsing `child()` onto its parent + // lets the spy below observe it. + const logger = serviceLocator.getLogger(); + vitest.spyOn(logger, 'child').mockReturnValue(logger); + vitest.spyOn(logger, 'info').mockImplementation((message: string, data?: Record | null) => { logged.push([message, data]); }); + const stats = new Statistics(); + stats.recordRequestStart(0); vitest.advanceTimersByTime(1); stats.recordRequestSuccess(0, 0); @@ -529,12 +537,16 @@ describe('Statistics', () => { productsFound: 'plenty', }); + // The statistics log through a child of the registered logger; collapsing `child()` onto its + // parent lets the spy below observe it. + const logger = serviceLocator.getLogger(); + vitest.spyOn(logger, 'child').mockReturnValue(logger); + const warningSpy = vitest.spyOn(logger, 'warning').mockImplementation(() => {}); + const stats = new Statistics({ id: 'corrupt-custom-stats', stateExtension: { deserialize: productsFound }, }); - // @ts-expect-error Accessing private prop - const warningSpy = vitest.spyOn(stats.log, 'warning').mockImplementation(() => {}); await stats.startCapturing(); diff --git a/test/core/impit_http_client.test.ts b/test/core/impit_http_client.test.ts index 58aa7648195f..7e278672ca20 100644 --- a/test/core/impit_http_client.test.ts +++ b/test/core/impit_http_client.test.ts @@ -14,20 +14,20 @@ describe('ImpitHttpClient', () => { vi.mocked(Impit).mockClear(); }); - test('reuses cached clients by default', () => { + test('reuses cached clients by default', async () => { const httpClient = new ImpitHttpClient(); - (httpClient as any).getClient({ proxyUrl: 'http://proxy.example' }); - (httpClient as any).getClient({ proxyUrl: 'http://proxy.example' }); + await httpClient.fetch(new Request('http://example.com'), { proxyUrl: 'http://proxy.example' }); + await httpClient.fetch(new Request('http://example.com'), { proxyUrl: 'http://proxy.example' }); expect(Impit).toHaveBeenCalledTimes(1); }); - test('creates a new client for each request when cacheClients is false', () => { + test('creates a new client for each request when cacheClients is false', async () => { const httpClient = new ImpitHttpClient({ cacheClients: false }); - (httpClient as any).getClient({ proxyUrl: 'http://proxy.example' }); - (httpClient as any).getClient({ proxyUrl: 'http://proxy.example' }); + await httpClient.fetch(new Request('http://example.com'), { proxyUrl: 'http://proxy.example' }); + await httpClient.fetch(new Request('http://example.com'), { proxyUrl: 'http://proxy.example' }); expect(Impit).toHaveBeenCalledTimes(2); }); diff --git a/test/core/request_list.test.ts b/test/core/request_list.test.ts index 38be9ec810c1..b94d4c5522b8 100644 --- a/test/core/request_list.test.ts +++ b/test/core/request_list.test.ts @@ -90,13 +90,12 @@ describe('RequestList', () => { await expect(requestList.markRequestAsHandled(requestObj)).rejects.toThrow(); await expect(requestList.fetchNextRequest()).rejects.toThrow(); - // @ts-expect-error private method - await requestList.initialize(); + const openedList = await RequestList.open(null, [{ url: 'https://example.com' }]); - await expect(requestList.checkReadiness()).resolves.not.toThrow(); - expect(() => requestList.getState()).not.toThrowError(); - await expect(requestList.fetchNextRequest()).resolves.not.toThrow(); - await expect(requestList.markRequestAsHandled(requestObj)).resolves.not.toThrow(); + await expect(openedList.checkReadiness()).resolves.not.toThrow(); + expect(() => openedList.getState()).not.toThrowError(); + await expect(openedList.fetchNextRequest()).resolves.not.toThrow(); + await expect(openedList.markRequestAsHandled(requestObj)).resolves.not.toThrow(); }); test('should correctly initialize itself', async () => { @@ -161,17 +160,21 @@ describe('RequestList', () => { }); test('should correctly load list from hosted files in correct order', async () => { - const spy = vitest.spyOn(RequestList.prototype as any, 'downloadListOfUrls'); const list1 = ['https://example.com', 'https://google.com', 'https://wired.com']; const list2 = ['https://another.com', 'https://page.com']; - spy.mockImplementationOnce(() => new Promise((resolve) => setTimeout(() => resolve(list1) as any, 100)) as any); - spy.mockResolvedValueOnce(list2); + mockHttpClient.sendRequest + .mockImplementationOnce(async () => { + await sleep(100); + return new Response(list1.join('\n')); + }) + .mockResolvedValueOnce(new Response(list2.join('\n'))); const requestList = await RequestList.open({ sources: [ { method: 'GET', requestsFromUrl: 'http://example.com/list-1' }, { method: 'POST', requestsFromUrl: 'http://example.com/list-2' }, ], + httpClient: mockHttpClient, }); expect(await requestList.fetchNextRequest()).toMatchObject({ method: 'GET', url: list1[0] }); @@ -180,9 +183,11 @@ describe('RequestList', () => { expect(await requestList.fetchNextRequest()).toMatchObject({ method: 'POST', url: list2[0] }); expect(await requestList.fetchNextRequest()).toMatchObject({ method: 'POST', url: list2[1] }); - expect(spy).toBeCalledTimes(2); - expect(spy).toBeCalledWith({ url: 'http://example.com/list-1', urlRegExp: undefined }); - expect(spy).toBeCalledWith({ url: 'http://example.com/list-2', urlRegExp: undefined }); + expect(mockHttpClient.sendRequest).toBeCalledTimes(2); + expect(mockHttpClient.sendRequest.mock.calls.map(([request]) => request.url)).toEqual([ + 'http://example.com/list-1', + 'http://example.com/list-2', + ]); }); test('should use regex parameter to parse urls', async () => { @@ -239,8 +244,7 @@ describe('RequestList', () => { }); test('should handle requestsFromUrl with no URLs', async () => { - const spy = vitest.spyOn(RequestList.prototype as any, 'downloadListOfUrls'); - spy.mockResolvedValueOnce([]); + mockHttpClient.sendRequest.mockResolvedValueOnce(new Response('')); const requestList = await RequestList.open({ sources: [ @@ -249,34 +253,36 @@ describe('RequestList', () => { requestsFromUrl: 'http://example.com/list-1', }, ], + httpClient: mockHttpClient, }); expect(await requestList.fetchNextRequest()).toBe(null); - expect(spy).toBeCalledTimes(1); - expect(spy).toBeCalledWith({ url: 'http://example.com/list-1', urlRegExp: undefined }); + expect(mockHttpClient.sendRequest).toBeCalledTimes(1); + expect(mockHttpClient.sendRequest.mock.calls[0][0].url).toBe('http://example.com/list-1'); }); test('should use the defined proxy server when using `requestsFromUrl`', async () => { const proxyUrls = ['http://proxyurl.usedforthe.download', 'http://another.proxy.url']; - const spy = vitest.spyOn(RequestList.prototype as any, 'downloadListOfUrls'); - spy.mockResolvedValue([]); - const proxyConfiguration = new ProxyConfiguration({ proxyUrls, }); - const requestList = await RequestList.open({ + await RequestList.open({ sources: [ { requestsFromUrl: 'http://example.com/list-1' }, { requestsFromUrl: 'http://example.com/list-2' }, { requestsFromUrl: 'http://example.com/list-3' }, ], proxyConfiguration, + httpClient: mockHttpClient, }); - expect(spy).not.toBeCalledWith(expect.not.objectContaining({ proxyUrl: expect.any(String) })); + expect(mockHttpClient.sendRequest).toBeCalledTimes(3); + for (const [, options] of mockHttpClient.sendRequest.mock.calls) { + expect(proxyUrls).toContain(options.proxyUrl); + } }); test('tracks in-progress requests through the crawl lifecycle', async () => { @@ -452,7 +458,6 @@ describe('RequestList', () => { const PERSIST_REQUESTS_KEY = 'some-key'; const getValueSpy = vitest.spyOn(KeyValueStore.prototype, 'getValue'); const setValueSpy = vitest.spyOn(KeyValueStore.prototype, 'setValue'); - const spy = vitest.spyOn(RequestList.prototype as any, 'downloadListOfUrls'); let persistedRequests: any; const opts = { @@ -463,10 +468,11 @@ describe('RequestList', () => { { url: 'https://example.com/5' }, ], persistRequestsKey: PERSIST_REQUESTS_KEY, + httpClient: mockHttpClient, }; const urlsFromTxt = ['http://example.com/3', 'http://example.com/4']; - spy.mockResolvedValueOnce(urlsFromTxt); + mockHttpClient.sendRequest.mockResolvedValueOnce(new Response(urlsFromTxt.join('\n'))); getValueSpy.mockResolvedValueOnce(null); setValueSpy.mockImplementationOnce(async (_key, value) => { @@ -479,8 +485,8 @@ describe('RequestList', () => { expect(requestList.requests).toHaveLength(5); expect(requests).toEqual(requestList.requests); - expect(spy).toBeCalledTimes(1); - expect(spy).toBeCalledWith({ url: 'http://example.com/list-urls.txt', urlRegExp: undefined }); + expect(mockHttpClient.sendRequest).toBeCalledTimes(1); + expect(mockHttpClient.sendRequest.mock.calls[0][0].url).toBe('http://example.com/list-urls.txt'); }); test('handles correctly inconsistent inProgress fields in state', async () => { diff --git a/test/core/session_pool/session_pool.test.ts b/test/core/session_pool/session_pool.test.ts index 2a2ec1294d83..85ce06fee14d 100644 --- a/test/core/session_pool/session_pool.test.ts +++ b/test/core/session_pool/session_pool.test.ts @@ -214,13 +214,10 @@ describe('SessionPool - testing session pool', () => { }); test('should create session', async () => { - // @ts-expect-error Accessing protected method - await sessionPool.ensureInitialized(); - // @ts-expect-error private symbol - await sessionPool.createSession(); + const created = await sessionPool.newSession(); const { sessions } = await sessionPool.getState(); expect(sessions).toHaveLength(1); - expect(sessions[0].id).toBeDefined(); + expect(sessions[0].id).toBe(created.id); }); describe('should persist state', () => { @@ -326,8 +323,8 @@ describe('SessionPool - testing session pool', () => { persistStateKeyValueStoreId, persistStateKey, }); - // @ts-expect-error Accessing protected method - await newSessionPool.ensureInitialized(); + // Any public use initializes the pool, which is what teardown then persists. + await newSessionPool.getState(); await newSessionPool.teardown(); @@ -358,8 +355,7 @@ describe('SessionPool - testing session pool', () => { it('should remove persist state event listener', async () => { const events = serviceLocator.getEventManager(); - // @ts-expect-error Accessing protected method - await sessionPool.ensureInitialized(); + await sessionPool.getState(); expect(events.listenerCount(EventType.PERSIST_STATE)).toEqual(1); await sessionPool.teardown(); expect(events.listenerCount(EventType.PERSIST_STATE)).toEqual(0); diff --git a/test/core/storages/request_queue.test.ts b/test/core/storages/request_queue.test.ts index 692902d6ac5f..a75f0b07f5e4 100644 --- a/test/core/storages/request_queue.test.ts +++ b/test/core/storages/request_queue.test.ts @@ -434,13 +434,16 @@ describe('RequestQueue with requestsFromUrl', () => { }); test('should correctly load list from hosted files in correct order', async () => { - const spy = vitest.spyOn(RequestQueue.prototype as any, 'downloadListOfUrls'); const list1 = ['https://example.com', 'https://google.com', 'https://wired.com']; const list2 = ['https://another.com', 'https://page.com']; - spy.mockImplementationOnce(() => new Promise((resolve) => setTimeout(() => resolve(list1) as any, 100)) as any); - spy.mockResolvedValueOnce(list2); - - const queue = await RequestQueue.open(); + mockHttpClient.sendRequest + .mockImplementationOnce(async () => { + await sleep(100); + return new Response(list1.join('\n')); + }) + .mockResolvedValueOnce(new Response(list2.join('\n'))); + + const queue = await RequestQueue.open(null, { httpClient: mockHttpClient }); await queue.addRequests([ { method: 'GET', requestsFromUrl: 'http://example.com/list-1' }, { method: 'POST', requestsFromUrl: 'http://example.com/list-2' }, @@ -452,9 +455,11 @@ describe('RequestQueue with requestsFromUrl', () => { expect(await queue.fetchNextRequest()).toMatchObject({ method: 'POST', url: list2[0] }); expect(await queue.fetchNextRequest()).toMatchObject({ method: 'POST', url: list2[1] }); - expect(spy).toHaveBeenCalledTimes(2); - expect(spy).toHaveBeenCalledWith({ url: 'http://example.com/list-1', urlRegExp: undefined }); - expect(spy).toHaveBeenCalledWith({ url: 'http://example.com/list-2', urlRegExp: undefined }); + expect(mockHttpClient.sendRequest).toHaveBeenCalledTimes(2); + expect(mockHttpClient.sendRequest.mock.calls.map(([request]) => request.url)).toEqual([ + 'http://example.com/list-1', + 'http://example.com/list-2', + ]); }); test('should use regex parameter to parse urls', async () => { @@ -510,10 +515,9 @@ describe('RequestQueue with requestsFromUrl', () => { }); test('should handle requestsFromUrl with no URLs', async () => { - const spy = vitest.spyOn(RequestQueue.prototype as any, 'downloadListOfUrls'); - spy.mockResolvedValueOnce([]); + mockHttpClient.sendRequest.mockResolvedValueOnce(new Response('')); - const queue = await RequestQueue.open(); + const queue = await RequestQueue.open(null, { httpClient: mockHttpClient }); await queue.addRequest({ method: 'GET', requestsFromUrl: 'http://example.com/list-1', @@ -521,28 +525,28 @@ describe('RequestQueue with requestsFromUrl', () => { expect(await queue.fetchNextRequest()).toBe(null); - expect(spy).toHaveBeenCalledTimes(1); - expect(spy).toHaveBeenCalledWith({ url: 'http://example.com/list-1', urlRegExp: undefined }); + expect(mockHttpClient.sendRequest).toHaveBeenCalledTimes(1); + expect(mockHttpClient.sendRequest.mock.calls[0][0].url).toBe('http://example.com/list-1'); }); test('should use the defined proxy server when using `requestsFromUrl`', async () => { const proxyUrls = ['http://proxyurl.usedforthe.download', 'http://another.proxy.url']; - const spy = vitest.spyOn(RequestQueue.prototype as any, 'downloadListOfUrls'); - spy.mockResolvedValue([]); - const proxyConfiguration = new ProxyConfiguration({ proxyUrls, }); - const queue = await RequestQueue.open(null, { proxyConfiguration }); + const queue = await RequestQueue.open(null, { proxyConfiguration, httpClient: mockHttpClient }); await queue.addRequests([ { requestsFromUrl: 'http://example.com/list-1' }, { requestsFromUrl: 'http://example.com/list-2' }, { requestsFromUrl: 'http://example.com/list-3' }, ]); - expect(spy).not.toHaveBeenCalledWith(expect.not.objectContaining({ proxyUrl: expect.any(String) })); + expect(mockHttpClient.sendRequest).toHaveBeenCalledTimes(3); + for (const [, options] of mockHttpClient.sendRequest.mock.calls) { + expect(proxyUrls).toContain(options.proxyUrl); + } }); }); diff --git a/test/core/storages/storage_transaction.test.ts b/test/core/storages/storage_transaction.test.ts index 63ed3f13556b..4388d456297b 100644 --- a/test/core/storages/storage_transaction.test.ts +++ b/test/core/storages/storage_transaction.test.ts @@ -556,20 +556,29 @@ describe('KeyValueStore in a transaction', () => { await store.setValue(`buffered-${i}`, { i }); } - // `bufferedJournalEntries()` is the O(journal) reduction. Reading 40 keys' values must not - // call it 40 times - the listing path builds one map and threads it through every per-key - // read. A regression to per-key derivation makes this scale with the key count. - const reduceSpy = vitest.spyOn(store as any, 'bufferedJournalEntries'); + // Deriving the buffered writes means one full scan of the journal. Reading 40 keys' values + // must not scan it 40 times - the listing path builds one map and threads it through every + // per-key read. A regression to per-key derivation makes this scale with the key count. + let journalScans = 0; + const { journal } = transaction; + const iterate = journal[Symbol.iterator].bind(journal); + Object.defineProperty(journal, Symbol.iterator, { + configurable: true, + value: () => { + journalScans++; + return iterate(); + }, + }); const values = await store.values(); expect(values).toHaveLength(40); // One reduction for the page listing, one shared across every record read: two, not forty. - // The lower bound matters too - zero calls would mean the buffered reads were skipped entirely. - expect(reduceSpy.mock.calls.length).toBeGreaterThanOrEqual(1); - expect(reduceSpy.mock.calls.length).toBeLessThanOrEqual(2); + // The lower bound matters too - zero scans would mean the buffered reads were skipped entirely. + expect(journalScans).toBeGreaterThanOrEqual(1); + expect(journalScans).toBeLessThanOrEqual(2); - reduceSpy.mockRestore(); + Reflect.deleteProperty(journal, Symbol.iterator); transaction.rollback(); }); }); diff --git a/test/stagehand-crawler/stagehand-controller.test.ts b/test/stagehand-crawler/stagehand-controller.test.ts index 43e34f31d5a5..4247286d508e 100644 --- a/test/stagehand-crawler/stagehand-controller.test.ts +++ b/test/stagehand-crawler/stagehand-controller.test.ts @@ -172,7 +172,7 @@ describe('StagehandController', () => { await expect((controller as any)._kill()).resolves.toBeUndefined(); }); - describe('_newPage', () => { + describe('newPage', () => { let mockCdpSession: any; let mockPage: any; @@ -196,7 +196,8 @@ describe('StagehandController', () => { const createController = () => { const controller = new StagehandController(mockPlugin, stagehandInstances); - (controller as any).browser = mockBrowser; + controller.assignBrowser(mockBrowser, {} as never); + controller.activate(); return controller; }; @@ -208,7 +209,7 @@ describe('StagehandController', () => { ++attempts < 3 ? undefined : { v3Page: true }, ); - const page = await (createController() as any)._newPage(); + const page = await createController().newPage(); expect(page).toBe(mockPage); expect(attempts).toBe(3); @@ -219,27 +220,29 @@ describe('StagehandController', () => { test('should detach the CDP session used to read the main frame id', async () => { mockStagehand.context.resolvePageByMainFrameId = vi.fn().mockReturnValue({ v3Page: true }); - await (createController() as any)._newPage(); + await createController().newPage(); expect(mockCdpSession.send).toHaveBeenCalledWith('Page.getFrameTree'); expect(mockCdpSession.detach).toHaveBeenCalled(); }); - test('should throw when Stagehand never registers the page', async () => { - const controller = createController(); + test('should close the page and fail when Stagehand never registers it', async () => { + // The page never resolves, so the wait has to run to its full deadline - fake timers keep that + // instant instead of stalling the test for the 10s default timeout. + vi.useFakeTimers(); - await expect((controller as any).waitForStagehandToRegisterPage(mockPage, 100)).rejects.toThrow( - 'Stagehand did not register the page within 100ms', - ); - }); + try { + const newPage = createController().newPage(); + const assertion = expect(newPage).rejects.toThrow( + 'Failed to create new page: Stagehand did not register the page within 10000ms', + ); - test('should close the page when Stagehand never registers it', async () => { - const controller = createController(); - vi.spyOn(controller as any, 'waitForStagehandToRegisterPage').mockRejectedValue( - new Error('not registered'), - ); + await vi.advanceTimersByTimeAsync(10_100); + await assertion; + } finally { + vi.useRealTimers(); + } - await expect((controller as any)._newPage()).rejects.toThrow('Failed to create new page: not registered'); expect(mockPage.close).toHaveBeenCalled(); }); });