fix: remove the never-failing ok assertion in test_no_chrome.py (#155)

[XEDAB]

Fixes #155

Description

run_cli in tests/test_no_chrome.py reported success when a run wrote no
files: with done == 0 it set ok to len(urls), so the ok assertion in
assert_browser_counts could never fail on the CLI path.

Removed:

• the vacuous assert result["ok"] == len(urls) from the shared helper
• the now-dead result / urls parameters from assert_browser_counts
• the fake ok / fail accounting in run_cli (its return value had no consumers)
• the now-unused import os

The ok check is genuine on the engine path — run_engine returns the engine's
real counters — so it was restored in test_engine_fuckingfast_only_launches_no_browser
and test_engine_mixed_batch_launches_one_shared_browser, where it still fails
if the engine drops a URL. The CLI tests no longer assert against a fabricated ok.

assert_browser_counts now only checks the browser counts, which the stubs
actually exercise.

Why this option: dropped the fabricated ok assertion rather than making
the stubs write files. The browser counts already carry the intent of these
tests, and an assertion that passes by construction means nothing. The genuine
engine-path check is kept because it is doing real work.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would change existing behavior)
• Documentation update
• Refactor / code cleanup
• Other:

Checklist

• I have tested my changes locally
• If this affects shared logic (extraction, download engine), I also
  applied the equivalent change to moon_cli.py
• I have kept the single-file architecture (no package split)
• I have not added new dependencies without justification in the PR
  description

Screenshots / logs (if applicable)

[LeyckerS]

@XEDAB — the reasoning in your description is right, the option you picked is the one I would have picked, and the writeup is exactly what I asked for. But the patch removes a live assertion along with the dead one, and that is my fault: #155 told you all of them were dead, and it was wrong.

assert_browser_counts has four callers, not three:

 87:  test_engine_fuckingfast_only_launches_no_browser   ← engine
 97:  test_engine_mixed_batch_launches_one_shared_browser ← engine
128:  test_cli_fuckingfast_only_launches_no_browser
136:  test_cli_mixed_batch_launches_one_shared_browser

The two engine tests reach it through run_engine, which does not synthesise anything:

metrics = engine.snapshot(0)["metrics"]
return {"ok": metrics["ok"], "fail": metrics["fail"]}

That is the engine's own counter. Measured on main, same fixtures, same URLs:

ENGINE_RESULT: {'ok': 4, 'fail': 0} len(urls)= 4

Real 4, not a substituted len(urls). So on the engine path assert result["ok"] == len(urls) fails the moment the engine drops a URL — it is doing exactly the job an assertion should. Compare the CLI path, which is where the probe in #155 gave FILES_WRITTEN: 0 and the value was fabricated.

One assertion, vacuous through two callers and live through two others. Deleting it from the shared helper fixes the first pair and silently removes coverage from the second — which is, awkwardly, the same category of problem this issue exists to fix.

Three corrections to #155, all mine: it said "three tests", it listed only CLI ones, and one of the names it gave — test_cli_datanodes_only_launches_one_browser — does not exist in the file at all. I have corrected the issue body. You built precisely what it described; the gap is between the issue and the repository, not between the issue and your patch.

What the fix needs to look like: keep the count assertion where the number is real, drop it where it is invented. Splitting the helper is the obvious route — the browser-count half is genuinely shared, the ok half is not — but if you see something cleaner, take it. run_cli returning None is right and so is dropping import os; I checked, os.listdir was its only use.

Everything else here stands: the diff is minimal, the description explains the choice, and the reasoning about a fourth assertion being worse than three is correct — it just happens that there were five, and one of them worked.

[LeyckerS]

This is the right fix. The assertion is kept inline where the number is real and dropped where it was invented, run_cli returns None, and import os goes with it.

I owed you a demonstration that the engine assertion still bites, so I ran it rather than asking you to. On your branch, with the extraction counter deliberately broken (moon_engine.py:270, dropping one self._inc("_ok")):

FAILED tests/test_no_chrome.py::test_engine_fuckingfast_only_launches_no_browser
FAILED tests/test_no_chrome.py::test_engine_mixed_batch_launches_one_shared_browser
2 failed, 6 passed

Both engine tests go red; the CLI tests stay green because they no longer assert on a fabricated count. That is exactly the split we wanted. Restored afterwards, pytest tests/ -q → 42 passed, and CI is green on all eight checks.

One thing I learned doing that, which corrects my own review again. I told you result["ok"] on the engine path means "every URL downloaded". It does not — run_engine passes "mode": "links", so the engine extracts and never downloads. Measured:

PROBE ok/fail/dl_done/dl_total: 4 0 0 4
PROBE files in _tmp_out: []

ok is 4 with dl_done at 0 and nothing on disk; the counter being asserted is the one at moon_engine.py:270, incremented per extracted link. So the assertion means "every URL was extracted and dispatched", which is precisely the property these browser-routing tests exist to protect. Your patch preserves the right thing — my description of what it was preserving was wrong. My first attempt at breaking it targeted the download branch and the tests stayed green, which is what sent me looking.

That also surfaced something separate and latent, which is not yours to fix here: tests/conftest.py:65 stubs download_file on moon_cli only, outside the for host in (moon_engine, moon_cli) loop just above it. The engine keeps the real one. It is harmless today only because the engine tests never download — flip run_engine to "mode": "download" and the suite starts making real network calls. Opening that separately.

Merging. Thank you for the patience across two wrong specifications from me — you found the bug, reported it precisely, and then fixed it properly on the second pass.