diff --git a/CLAUDE.md b/CLAUDE.md index 6fdeff2..d32bfb6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -105,22 +105,16 @@ ruff check . внутри `output_root`), а каждый файл сразу после скачивания парсится/переносится в свой `` через `write_dataset`/ `finalize_raw`. - - Перед вводом новой фразы (`_collect_one`) снимается снапшот текста первой - строки таблицы; после `_set_phrase`/`_set_region` даётся короткое (3s, - `_wait_for(..., required=False)`) окно на то, чтобы он изменился. Это - **мягкая подсказка, не гейт**: две разные (например, близкие) фразы могут легально - дать одинаковую первую строку или обе — пустую таблицу, так что таймаут - здесь не бросает исключение и не роняет фразу — просто ничего не - подтверждает. `_set_phrase` сама проверяет URL (`words=`) и кнопку - скачивания, не содержимое таблицы; без этой подсказки риск тихо скачать - данные предыдущей фразы под именем следующей был бы выше, но полностью - он не устранён — гейт умышленно не строгий. Проверено на живом - авторизованном Chrome: близкая пара фраз («ремонт квартир» / - «ремонт квартир цена») собралась без ложного отказа за ~13с суммарно - (раньше строгий гейт на этой паре давал ~45с простоя и - `InterfaceChangedError`), и данные обеих фраз в `dynamics`/`regions` - корректно различаются — устаревания не произошло ни разу за несколько - прогонов. + - Перед вводом следующей фразы (`_collect_one`) используется снапшот текста + первой строки, снятый в предыдущем табличном виде **до** перехода на + финальную карту `regions`; карта строк не содержит. После `_set_phrase`/ + `_set_region` даётся короткое (3s, `_wait_for(..., required=False)`) окно + на изменение текста. Это **мягкая подсказка, не гейт**: две разные + (например, близкие) фразы могут легально дать одинаковую первую строку + или обе — пустую таблицу, поэтому таймаут не бросает исключение и не + роняет фразу — просто ничего не подтверждает. `_set_phrase` сама + проверяет URL (`words=`) и кнопку скачивания, не содержимое таблицы. + - Порядок «скачать → распарсить → записать parquet → убрать raw» обязателен: при любой ошибке в этом блоке (не только `CsvFormatError` — тот же инвариант держится и для сбоя `write_dataset`/`finalize_raw`) скачанный @@ -197,44 +191,31 @@ ruff check . молча скачивает CSV только с заголовком. `regions` (карта) от этого условия освобождён — у него в DOM вообще нет строк таблицы, и гейт по ним завис бы навсегда. - - **Гейт по устаревшим строкам таблицы (issue #3).** Наличия строк - (`length > 0`) недостаточно: они могут быть строками *предыдущего* вида, - ещё не смытыми перерисовкой. Перед кликом по вкладке `_select_view` - проверяет, активна ли уже целевая вкладка (`_is_view_active`, `checked` - на её radio-input **до** клика). Если да — значит клик по ней ничего не - переключает (то же самое представление уже открыто), и требовать смены - содержимого таблицы было бы неверно: клик по уже активной вкладке - структурно не обязан её перерисовывать. Так происходит систематически - для самого первого представления каждой фразы: цикл видов в - `_collect_one` всегда начинается с `top_popular`, а Wordstat обычно уже - открывает именно эту вкладку сразу после поиска фразы — первая версия - этого гейта (безусловный снапшот + жёсткое требование смены) вешала - ровно это первое представление каждого прогона до таймаута (найдено на - втором раунде ревью, до попадания в прод). Только когда целевая вкладка - была неактивна до клика, `_select_view` снимает снапшот текста первой - строки таблицы (`_table_snapshot`, тот же приём, что и в `_collect_one` - для смены фразы) и требует, чтобы содержимое таблицы **отличалось** от - этого снапшота — в отличие от мягкой подсказки при смене фразы, здесь - это жёсткий гейт для настоящих переключений: таймаут тут — настоящий - `InterfaceChangedError`, а не молчаливое "не подтвердилось, но и не - страшно". `regions` (карта) освобождён и от этого условия, и от - проверки активности — по той же причине, что и от `length > 0`. - Проверено на живом Wordstat (CDP `9223`): одиночная фраза и batch из - двух фраз собираются без зависаний на первом представлении, `regions`/ - `dynamics` корректно различаются между фразами. - - **Историческая проблема (issue #3), найдена ручным прогоном на живом - Wordstat**: CSV для `top_popular`/`top_related` (представления «Что - искали со словом…» и «Похожие запросы») иногда скачивался с одной - строкой — только заголовком, без данных, хотя `_select_view` дожидался - `TABLE_ROW_SELECTOR.length > 0` перед кликом по «Скачать». Причина — - `TABLE_ROW_SELECTOR` ловил наличие **каких-то** строк, не различая - старые (от предыдущего вида) и новые. Гейт по изменению содержимого - таблицы выше адресует эту причину для настоящих переключений вида; - `dtypes`/`row_count: 0` в манифесте по-прежнему честно отражают пустой - CSV, если он всё же случится (подтверждено живым прогоном — оба - табличных представления для нескольких проверенных фраз стабильно - приходят пустыми, без строк вообще, что похоже на отдельное, - самостоятельное поведение Wordstat для этих фраз, а не на гонку). + - **Готовность вида и содержимое таблицы (issue #3/#13).** Единственный + жёсткий гейт `_select_view` — активный `checked` radio-маркер, кнопка + скачивания и, только для `top_popular`/`top_related`/`dynamics`, хотя бы + одна строка `TABLE_ROW_SELECTOR`. Последнее не даёт нажать «Скачать» в + известном окне, когда radio уже сменился, а таблица ещё не отрисована и + CSV выходит только с заголовком. `regions` (карта) от строки освобождён: + в его DOM таблицы нет. + + Сравнение текста таблицы до/после клика — полезная улика перерисовки, но + **никогда не жёсткий гейт**: разные вкладки законно могут иметь одинаковую + первую строку (или одинаково пустое содержимое). После жёсткого гейта + `_select_view` делает эту проверку отдельным + `_wait_for(..., required=False)`; таймаут лишь не подтверждает сигнал и + не отклоняет фразу. Поэтому `_is_view_active` больше не нужен: клик по + уже активной вкладке и обычное переключение используют один безопасный + гейт. Ретрай клика остаётся: две попытки делят общий timeout пополам. + + - **Историческая проблема пустых CSV (issue #3).** На живом Wordstat + `top_popular`/`top_related` иногда скачивались только с заголовком, когда + DOM ещё показывал переходное состояние. Не ослабляйте обязательное + `TABLE_ROW_SELECTOR.length > 0` для табличных видов: при ручном прогоне + проверяйте, что `row_count` у всех четырёх экспортов не равен нулю. + Контентное сравнение не заменяет этот гейт и не должно снова стать + строгим. + - **`storage.py`** — файловая изоляция запусков: каждый вызов `collect` создаёт новый неразрушающий каталог diff --git a/src/wordstat/collector.py b/src/wordstat/collector.py index 543a0d0..c08fe60 100644 --- a/src/wordstat/collector.py +++ b/src/wordstat/collector.py @@ -79,6 +79,7 @@ def __init__( self.output_root = output_root self.timeout_seconds = timeout_seconds self.keep_raw = keep_raw + self._previous_table_snapshot: str | None = None async def collect( self, phrase: str, region: str = "Россия", resume_directory: Path | None = None @@ -261,15 +262,13 @@ async def _collect_one( manifest = None manifest_path = run_directory / "manifest.json" pending_views = list(WordstatView) - # In a batch, the previous phrase's table can still be sitting in the - # DOM when _set_phrase's own waits (new `words=` in the URL, download - # button present) are satisfied — those don't check the table itself. - # Snapshot it before switching phrases so we can give Wordstat a - # short extra moment to re-render for the new phrase. This is a best - # effort nudge, not a hard gate: two different (e.g. related) phrases - # can legally produce the same first row, or an empty table, so a - # timeout here must never fail the phrase — see _wait_for(required=False). - previous_table = await self._table_snapshot(page) + # The preceding complete view loop ends on REGIONS, whose map has no + # table. Keep the last table snapshot while a table view is visible + # so the next phrase gets the best-effort re-render nudge. Content + # equality is only evidence, never a gate: related phrases can + # legally have the same first row (or no row). + previous_table = self._previous_table_snapshot + self._previous_table_snapshot = None await self._set_phrase(page, phrase) if set_region: # Only until region is actually applied — collect_many tracks @@ -327,9 +326,7 @@ async def _collect_one( # Doing this after _set_phrase (not before) is required: the # page is still on the previous phrase/tab until _set_phrase # returns. - manifest = manifest.model_copy( - update={"source_url": await page.get_url(), "updated_at": datetime.now(UTC)} - ) + manifest = manifest.model_copy(update={"source_url": await page.get_url(), "updated_at": datetime.now(UTC)}) write_manifest(manifest_path, manifest) for view in pending_views: @@ -353,6 +350,8 @@ async def _collect_one( if source.exists(): source.replace(run_directory / source.name) raise + if view != WordstatView.REGIONS: + self._previous_table_snapshot = await self._table_snapshot(page) export = ExportSummary( view=view, file=data_path.name, @@ -480,38 +479,15 @@ async def _select_view(self, page, selector: str, view: WordstatView) -> None: # (checked on the live page). Retry the click once if the marker # does not become active before the normal wait timeout. # - # The radio flipping to checked does not mean the table has - # repainted yet: for the three table-based views, the download - # button and an active marker can both be true while the DOM still - # shows the *previous* view's rows (or none), so downloading right - # then silently produces a header-only CSV (row_count: 0 — see - # issue #3 and the known-issue note in CLAUDE.md). REGIONS (the - # map) has no table rows at all, so it is exempt from this extra - # gate — requiring row presence there would hang forever. - # - # Row *presence* alone doesn't prove the rows are the new view's, - # not stale leftovers from the previous one — the same ambiguity - # `_collect_one` already handles for phrase switches via a table - # snapshot. A first pass at this (see PR #7 history) snapshotted - # the table unconditionally and required it change after the click - # — but the view loop always starts with TOP_POPULAR, and Wordstat - # is normally already showing that exact view right after a search, - # so clicking its own already-active tab has no reason to change - # the table at all: the gate was unconditionally unsatisfiable on - # the very first view of every phrase (found in review, round 2). + # For table-based views, the radio and download button can become + # ready before the table itself renders. Row presence therefore stays + # in the hard gate: without it Wordstat can download a header-only + # CSV. REGIONS is a map with no table rows, so it is exempt. # - # Fix: read whether the target tab is ALREADY checked before - # clicking. If so, Wordstat isn't switching views at all — the - # click is a same-view no-op — so only the plain checked+download - # (+rows for table views) predicate applies, with no content-delta - # requirement. Only when the target tab is genuinely inactive - # before the click do we snapshot the table and require its content - # to differ, because that is the only case where "did the view - # actually repaint" is a real question. - was_already_active = view != WordstatView.REGIONS and await self._is_view_active(page, selector) - previous_table = ( - await self._table_snapshot(page) if view != WordstatView.REGIONS and not was_already_active else None - ) + # A change in table text is useful corroborating evidence, but cannot + # be a hard gate: different views can legitimately have the same + # first row. Check it separately and best-effort after the hard gate. + previous_table = await self._table_snapshot(page) if view != WordstatView.REGIONS else None target_selector = json.dumps(selector) ready_expression = f"""() => {{ const label = document.querySelector({target_selector}); @@ -523,16 +499,7 @@ async def _select_view(self, page, selector: str, view: WordstatView) -> None: ready_expression += ( f"\n && document.querySelectorAll({json.dumps(TABLE_ROW_SELECTOR)}).length > 0" ) - if not was_already_active: - ready_expression += ( - f"\n && document.querySelector({json.dumps(TABLE_ROW_SELECTOR)})?.textContent" - f" !== {json.dumps(previous_table)};" - ) - else: - ready_expression += ";" - else: - ready_expression += ";" - ready_expression += "\n }" + ready_expression += ";\n }" # Two attempts must not cost double the configured timeout budget # (issue found in review): split it so the worst case across both # attempts still matches self.timeout_seconds, not 2x it. @@ -541,20 +508,18 @@ async def _select_view(self, page, selector: str, view: WordstatView) -> None: await self._click(page, selector) try: await self._wait_for(page, ready_expression, seconds=attempt_seconds) - return + break except InterfaceChangedError: if attempt == 1: raise - - async def _is_view_active(self, page, selector: str) -> bool: - target_selector = json.dumps(selector) - result = await page.evaluate(f"""() => {{ - const label = document.querySelector({target_selector}); - const input = label?.querySelector('input') - ?? (label?.htmlFor ? document.getElementById(label.htmlFor) : null); - return Boolean(input?.checked); - }}""") - return bool(result) + if previous_table is not None: + await self._wait_for( + page, + f"() => document.querySelector({json.dumps(TABLE_ROW_SELECTOR)})?.textContent" + f" !== {json.dumps(previous_table)}", + seconds=3.0, + required=False, + ) async def _table_snapshot(self, page) -> str | None: return await page.evaluate( @@ -625,12 +590,11 @@ async def _click_by_text(self, page, selector: str, text: str) -> None: async def _wait_for(self, page, expression: str, seconds: float | None = None, required: bool = True) -> None: """Poll `expression` until it's true, or give up after the deadline. - `required=False` (used for the staleness nudge in _collect_one) makes - the timeout a no-op instead of a failure: two different phrases can - legally produce the same table content, so that signal is a useful - hint but not a reliable gate — a timeout there just means the nudge - could not confirm anything either way, and the caller proceeds - regardless. + `required=False` (used for table-content corroboration in + _collect_one and _select_view) makes the timeout a no-op instead of a + failure: different phrases or views can legally produce the same + table content, so that signal is a useful hint but not a reliable + gate — a timeout just means it could not confirm anything either way. """ deadline = time.monotonic() + (seconds if seconds is not None else self.timeout_seconds) while time.monotonic() < deadline: diff --git a/tests/test_collector_batch.py b/tests/test_collector_batch.py index e7f8f7e..2c04ae2 100644 --- a/tests/test_collector_batch.py +++ b/tests/test_collector_batch.py @@ -420,6 +420,57 @@ def _write_view_csv(path, phrase): path.write_text(f"Запрос;Показов\n{phrase};100\n", encoding="cp1251") +def test_collect_one_preserves_a_table_snapshot_for_the_next_phrase(monkeypatch, tmp_path): + """The map is last, so the next phrase must not snapshot it as None.""" + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + waits = [] + snapshots = iter( + ["popular first", "related first", "dynamics first", "popular second", "related second", "dynamics second"] + ) + download_count = 0 + + async def noop(*args, **kwargs): + pass + + async def snapshot(self, page): + return next(snapshots) + + async def wait(self, page, expression, seconds=None, required=True): + waits.append((expression, seconds, required)) + + async def download(self, page, session, directory): + nonlocal download_count + download_count += 1 + source = directory / f"export-{download_count}.csv" + _write_view_csv(source, "тест") + return source + + monkeypatch.setattr(WordstatCollector, "_assert_authenticated", noop) + monkeypatch.setattr(WordstatCollector, "_set_phrase", noop) + monkeypatch.setattr(WordstatCollector, "_set_region", noop) + monkeypatch.setattr(WordstatCollector, "_select_view", noop) + monkeypatch.setattr(WordstatCollector, "_table_snapshot", snapshot) + monkeypatch.setattr(WordstatCollector, "_wait_for", wait) + monkeypatch.setattr(WordstatCollector, "_download_current_view", download) + + collector = WordstatCollector("cdp", tmp_path) + page = _FakePage() + session = _FakeSession() + asyncio.run(collector._collect_one(page, session, downloads_path, "первая", "Россия", set_region=False)) + asyncio.run(collector._collect_one(page, session, downloads_path, "вторая", "Россия", set_region=False)) + + assert waits == [ + ( + '() => document.querySelector(".table__wrapper tbody tr")?.textContent !== "dynamics first"', + 3.0, + False, + ) + ] + assert collector._previous_table_snapshot == "dynamics second" + + def test_collect_one_rescues_the_csv_into_the_run_directory_on_write_failure(monkeypatch, tmp_path): """A4: the CSV rescue must trigger for ANY failure past parsing (e.g. a write_dataset/pyarrow error), not just CsvFormatError — and the original diff --git a/tests/test_collector_view.py b/tests/test_collector_view.py index 2982ae0..f8bda31 100644 --- a/tests/test_collector_view.py +++ b/tests/test_collector_view.py @@ -24,9 +24,7 @@ async def wait(self, page, expression, seconds=None, required=True): monkeypatch.setattr(WordstatCollector, "_click", click) monkeypatch.setattr(WordstatCollector, "_wait_for", wait) - asyncio.run( - WordstatCollector("cdp", tmp_path)._select_view(object(), "label[for='map']", WordstatView.REGIONS) - ) + asyncio.run(WordstatCollector("cdp", tmp_path)._select_view(object(), "label[for='map']", WordstatView.REGIONS)) assert clicks == ["label[for='map']", "label[for='map']"] assert len(waits) == 2 @@ -46,13 +44,9 @@ async def wait(self, page, expression, seconds=None, required=True): async def snapshot(self, page): return None - async def is_active(self, page, selector): - return False - monkeypatch.setattr(WordstatCollector, "_click", click) monkeypatch.setattr(WordstatCollector, "_wait_for", wait) monkeypatch.setattr(WordstatCollector, "_table_snapshot", snapshot) - monkeypatch.setattr(WordstatCollector, "_is_view_active", is_active) with pytest.raises(InterfaceChangedError, match="view did not change"): asyncio.run( @@ -78,13 +72,9 @@ async def wait(self, page, expression, seconds=None, required=True): async def snapshot(self, page): return None - async def is_active(self, page, selector): - return False - monkeypatch.setattr(WordstatCollector, "_click", click) monkeypatch.setattr(WordstatCollector, "_wait_for", wait) monkeypatch.setattr(WordstatCollector, "_table_snapshot", snapshot) - monkeypatch.setattr(WordstatCollector, "_is_view_active", is_active) collector = WordstatCollector("cdp", tmp_path, timeout_seconds=45.0) with pytest.raises(InterfaceChangedError): @@ -112,13 +102,9 @@ async def wait(self, page, expression, seconds=None, required=True): async def snapshot(self, page): return "previous row text" - async def is_active(self, page, selector): - return False - monkeypatch.setattr(WordstatCollector, "_click", click) monkeypatch.setattr(WordstatCollector, "_wait_for", wait) monkeypatch.setattr(WordstatCollector, "_table_snapshot", snapshot) - monkeypatch.setattr(WordstatCollector, "_is_view_active", is_active) asyncio.run( WordstatCollector("cdp", tmp_path)._select_view(object(), "label[for='table']", WordstatView.TOP_POPULAR) @@ -127,78 +113,39 @@ async def is_active(self, page, selector): assert ".table__wrapper" in waits[0] -def test_select_view_requires_table_content_to_change_when_switching_views(monkeypatch, tmp_path): - # Regression guard (found in review, issue #3's own known-issue note): - # `checked` + download-button + row-count>0 can all be true while the - # DOM still shows the *previous* view's rows, silently downloading a - # header-only or wrong-view CSV. When genuinely switching from a - # different, currently-inactive view, the readiness predicate must - # also require the table's content to differ from what it was right - # before the click, mirroring the staleness snapshot _collect_one - # already uses for phrase switches. - waits = [] - - async def click(self, page, selector): - pass - - async def wait(self, page, expression, seconds=None, required=True): - waits.append(expression) - - async def snapshot(self, page): - return "stale row from the previous view" - - async def is_active(self, page, selector): - return False - - monkeypatch.setattr(WordstatCollector, "_click", click) - monkeypatch.setattr(WordstatCollector, "_wait_for", wait) - monkeypatch.setattr(WordstatCollector, "_table_snapshot", snapshot) - monkeypatch.setattr(WordstatCollector, "_is_view_active", is_active) - - asyncio.run( - WordstatCollector("cdp", tmp_path)._select_view(object(), "label[for='table']", WordstatView.TOP_POPULAR) - ) - - assert "stale row from the previous view" in waits[0] - assert "!==" in waits[0] - - -def test_select_view_skips_content_change_requirement_when_already_active(monkeypatch, tmp_path): - # Regression guard (found in review, round 2): the view loop always - # starts with TOP_POPULAR, and Wordstat is normally already showing - # that exact view right after a phrase search. Clicking an - # already-active tab does not change its own table content, so the - # content-delta gate must not apply in that case — otherwise the very - # first view of every phrase would hang until InterfaceChangedError. +def test_select_view_keeps_content_change_out_of_the_hard_gate(monkeypatch, tmp_path): + # Table text is helpful evidence of a repaint, but different views may + # legitimately have equal first rows. The required gate must remain only + # checked + download + rows; the content check follows as best-effort. waits = [] - snapshot_calls = 0 async def click(self, page, selector): pass async def wait(self, page, expression, seconds=None, required=True): - waits.append(expression) + waits.append((expression, seconds, required)) async def snapshot(self, page): - nonlocal snapshot_calls - snapshot_calls += 1 - return "same row either way" - - async def is_active(self, page, selector): - return True + return "same first row in two views" monkeypatch.setattr(WordstatCollector, "_click", click) monkeypatch.setattr(WordstatCollector, "_wait_for", wait) monkeypatch.setattr(WordstatCollector, "_table_snapshot", snapshot) - monkeypatch.setattr(WordstatCollector, "_is_view_active", is_active) asyncio.run( WordstatCollector("cdp", tmp_path)._select_view(object(), "label[for='table']", WordstatView.TOP_POPULAR) ) - assert ".table__wrapper" in waits[0] - assert "!==" not in waits[0] - assert snapshot_calls == 0 + hard_expression, _, hard_required = waits[0] + soft_expression, soft_seconds, soft_required = waits[1] + assert ".table__wrapper" in hard_expression + assert "same first row in two views" not in hard_expression + assert "!==" not in hard_expression + assert hard_required is True + assert "same first row in two views" in soft_expression + assert "!==" in soft_expression + assert soft_seconds == 3.0 + assert soft_required is False def test_select_view_does_not_wait_for_table_rows_on_map_view(monkeypatch, tmp_path): @@ -216,8 +163,6 @@ async def wait(self, page, expression, seconds=None, required=True): monkeypatch.setattr(WordstatCollector, "_click", click) monkeypatch.setattr(WordstatCollector, "_wait_for", wait) - asyncio.run( - WordstatCollector("cdp", tmp_path)._select_view(object(), "label[for='map']", WordstatView.REGIONS) - ) + asyncio.run(WordstatCollector("cdp", tmp_path)._select_view(object(), "label[for='map']", WordstatView.REGIONS)) assert ".table__wrapper" not in waits[0]