Skip to content

test: Deflake adaptive crawler timeout test against slow CI browser navigation - #2207

Closed
vdusek wants to merge 2 commits into
masterfrom
test/deflake-adaptive-sub-crawler-timeout
Closed

vdusek wants to merge 2 commits into
masterfrom
test/deflake-adaptive-sub-crawler-timeout

Conversation

@vdusek

@vdusek vdusek commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

test_adaptive_playwright_crawler_timeout_in_sub_crawler fails intermittently on Windows CI with
Expected 'browser_handler' to be called once. Called 0 times.
(example run).
The fallback browser request navigates under the Playwright sub-crawler's own navigation_timeout
(default 1 minute) - a budget separate from the request handler timeout the test already relaxes to
120s. On a saturated runner (8 xdist workers, each driving Chromium) page.goto can exceed it, and
with max_request_retries=0 that single slow navigation fails the only attempt before the handler
ever runs.

The test now grants navigation the same 120s ceiling via playwright_crawler_specific_kwargs. To
pass that type-safely, the missing navigation_timeout key is added to
_PlaywrightCrawlerAdditionalOptions - typing only, PlaywrightCrawler.__init__ already accepts it.

Verified by deterministic fault injection (a playwright-only pre-navigation delay consuming the
shared navigation budget): a 70s navigation-phase delay reproduces the exact CI failure under the
default ceiling and passes under the 120s one. After the fix, 0/96 failures across 8 concurrent
Chromium-saturated lanes, and the adaptive + playwright test modules pass under -n auto. The
assertions are unchanged.

✍️ Drafted by Claude Code

@vdusek vdusek added t-tooling Issues with this label are in the ownership of the tooling team. adhoc Ad-hoc unplanned task added during the sprint. labels Sep 1, 2026
@vdusek vdusek self-assigned this Sep 1, 2026
@github-actions github-actions Bot added this to the 148th sprint - Tooling team milestone Sep 1, 2026
@github-actions github-actions Bot added the tested Temporary label used only programatically for some analytics. label Sep 1, 2026
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.73%. Comparing base (7e71d70) to head (c7df284).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2207      +/-   ##
==========================================
- Coverage   93.74%   93.73%   -0.01%     
==========================================
  Files         181      181              
  Lines       12852    12854       +2     
==========================================
+ Hits        12048    12049       +1     
- Misses        804      805       +1     
Flag Coverage Δ
unit 93.73% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vdusek vdusek closed this Sep 7, 2026
vdusek added a commit that referenced this pull request Sep 8, 2026
…2221)

Two adaptive crawler tests flake on the Windows CI shard for the same
reason. This supersedes #2207, which fixed one of them from a base that
predates #2203.

The browser sub crawler navigates under `PlaywrightCrawler`'s own
`navigation_timeout`, a 60s `SharedTimeout` shared by the
pre/post-navigation hooks and `goto`. That budget is separate from
`request_handler_timeout`, because `BasicCrawler` applies the handler
timeout only around the router call. So a cold Chromium launch on a
saturated runner can blow the navigation budget while the handler
timeout still has minutes left, and relaxing the handler timeout does
nothing about it.

- `test_adaptive_crawling_statistics` fails with `assert 3 == 1`
([example
run](https://github.com/apify/crawlee-python/actions/runs/34120120329/job/101736011265)).
When navigation exceeds the 60s budget, `BasicCrawler` retries the whole
request, which is correct behavior, and every adaptive counter
increments again. The failing run timed out twice before succeeding: 60
+ 60 + 17s matches its 146s crawler runtime. It now passes a 5 minute
`navigation_timeout`, the same ceiling #2203 already gave its handler.
- `test_adaptive_playwright_crawler_timeout_in_sub_crawler` fails with
`Expected 'browser_handler' to be called once. Called 0 times.`
([example
run](https://github.com/apify/crawlee-python/actions/runs/33467016711/job/99728873095)).
It sets `max_request_retries=0`, so one slow navigation fails the only
attempt before the handler ever runs. It now passes a 120s
`navigation_timeout`, matching the handler timeout it already relaxes.

Both keep their existing assertions, so a real double-counting or
missed-increment regression still fails them. Passing the key needs
`navigation_timeout` in `_PlaywrightCrawlerAdditionalOptions`;
`PlaywrightCrawler.__init__` already accepts it and `ty` reports
`invalid-key` without it.

Verified with deterministic fault injection in both cases. For the
statistics test, monkeypatching `Page.goto` to stall the first two
navigations by 70s reproduces the exact `assert 3 == 1` with two retries
under the 60s ceiling, and passes in 71s under the 5 minute one with all
counters at 1; plus 0 failures in 39 runs of that test alone and 31/31
for the whole file serially. For the sub crawler timeout test, a
playwright-only pre-navigation delay of 70s consumes the shared budget
and reproduces the CI failure under the default ceiling, passing under
120s, with 0/96 failures across 8 concurrent Chromium-saturated lanes
afterwards. The adaptive module passes under `-n auto` in 6 of 7 runs;
the exception hit an unrelated `Errno 98` in the uvicorn test server
fixture, caused by running several pytest processes at once locally.

*✍️ Drafted by Claude Code*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants