eschutho commented on PR #42118: URL: https://github.com/apache/superset/pull/42118#issuecomment-5064287617
Thanks for the review, @rusackas — went through the bot threads: - **CodeAnt's "stale `remaining_budget`" finding (Major, both threads)**: confirmed real. `remaining_budget` was computed once per tile before the scroll-settle sleep, then reused (stale) to cap the readiness wait after it, letting each tile overrun the budget by up to one settle interval. Fixed in bcb5957: recompute elapsed/remaining budget after the settle sleep (and re-check for exhaustion there too) before deriving the readiness timeout. Added a regression test (`test_readiness_wait_uses_budget_recomputed_after_scroll_settle`) that fails against the old code. - **Bito's "Documentation contradicts Celery API" (filtered/muted, screenshot_utils.py:96)**: this turned out to be a real bug, not just docs — `current_task.request.timelimit` is `(time_limit, soft_time_limit)`, i.e. `(hard, soft)`, confirmed against `celery.app.amqp`/`celery.app.task.Context`/`celery.worker.strategy` source. My original unpacking had it backwards. Joe independently caught and fixed the same thing in 245cb8b before I got to it — my rebase picked that up, and I aligned the mock/tests to match Celery's real tuple order (not just my implementation) so a regression here would fail the test. - **Bito's "test coverage gap for `log_context`" (webdriver.py)**: added `test_tiled_path_forwards_non_none_log_context`, asserting a non-`None` `log_context` passed to `WebDriverPlaywright.get_screenshot()` reaches `take_tiled_screenshot()` unchanged. - **CodeAnt's type-annotation nits** on test fixtures/methods: leaving these as-is per your comment that they're not necessary. - Pre-commit / ruff-format: clean against the pinned ruff 0.9.7 (verified directly, not just my sandbox's older local ruff, which briefly fought your formatting fix in the other direction — reverted before pushing). Pushed in bcb5957ff9. CI failures I see now (`playwright-tests-experimental`) look unrelated — an embedded-dashboard pivot-table test, not touching this code path. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
