test_no_chrome.py reports success when the run produced no files, so the ok assertion cannot fail

[LeyckerS]

Found by @XEDAB while working on #97 — their branch is the first thing that ever made this assertion do work, and it failed immediately. Details and the measurement are in this thread.

The problem

tests/test_no_chrome.py:50:

def run_cli(urls, retries=1) -> dict:
    with tempfile.TemporaryDirectory() as out:
        asyncio.run(moon_cli.run(urls, out, 4, 4, retries, "proxies.txt"))
        done = len(os.listdir(out))
    return {"ok": len(urls) if done == 0 else done, "fail": 0}

If the run writes no files, done == 0 and ok becomes len(urls). assert_browser_counts then asserts:

assert result["ok"] == len(urls)

which is len(urls) == len(urls). The zero-file case — the one worth catching — is reported as a complete success.

It is the case that actually happens

Measured on current main with a temporary probe using the same fixtures and the same URLs as test_cli_mixed_batch_launches_one_shared_browser:

FILES_WRITTEN: 0 []

So that assertion has never been able to fail on any run of this suite. The open_browser / playwright / close_browser counts above it are real and do their job; only the ok line is inert.

Three tests share assert_browser_counts and all three inherit this:

• test_cli_fuckingfast_only_launches_no_browser
• test_cli_mixed_batch_launches_one_shared_browser
• test_cli_datanodes_only_launches_one_browser

The decision to make

There is a real choice here and I do not want to prescribe the wrong one again:

1. Make the stub write files, so done is a true count and the assertion means "every URL produced a file". Strongest, but it puts the stubs on the hook for producing believable output and may need the download layer stubbed further down than it currently is.
2. Drop the ok assertion and replace it with something the stub actually guarantees — the browser counts already carry the intent of these tests, and a fourth assertion that cannot fail is worse than three that can.

Option 2 is smaller and honest. Option 1 is more valuable if the stub can support it without becoming a second implementation of the downloader. Whoever takes this should pick one and say why in the PR body; either is acceptable, an assertion that silently cannot fail is not.

Done when

• No assertion in test_no_chrome.py passes by construction
• If the ok check survives in any form, it fails when the run produces fewer files than URLs — demonstrate this, e.g. by breaking something locally and showing the test goes red
• pytest tests/ -q green

Needs no Windows machine. The stubs replace Chrome and the network, so this runs anywhere.

[LeyckerS]

Reserved for @XEDAB, who found this while working on #97.

I tried to set the assignee and GitHub refused it: the field only accepts collaborators, or accounts that have commented on this specific issue. @XEDAB — comment here with anything at all and I will assign it properly. Until then this note holds it, and nobody else should open a pull request against it.

[LeyckerS]

Correcting this issue — the scope I gave was wrong in three ways, and @XEDAB built to it in good faith in #157.

I wrote that "three tests share assert_browser_counts" and listed three CLI tests. All three claims are wrong:

1. It has four callers, not three — test_no_chrome.py lines 87, 97, 128, 136.

2. Two of them are engine tests, and on that path the assertion is live, not vacuous. run_engine returns the engine's real counters:

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

   Measured: ENGINE_RESULT: {'ok': 4, 'fail': 0} len(urls)= 4. A real 4, not a substituted one — so assert result["ok"] == len(urls) fails there the moment the engine loses a URL.

3. test_cli_datanodes_only_launches_one_browser, which I named, does not exist. I did not check the file before listing it.

The corrected problem: one assertion, reached by four tests, fabricated through the two run_cli callers and genuine through the two run_engine ones. run_cli returns {"ok": len(urls) if done == 0 else done} and the stubs write no files (FILES_WRITTEN: 0), so on that path it compares a number with itself.

The corrected work: keep the assertion where the number is real, remove it where it is invented — most likely by splitting the helper, since the browser-count half is legitimately shared and the ok half is not. Do not simply delete it from the shared helper: that trades a fake assertion for the loss of a working one.

Done when

• The CLI tests no longer assert against a fabricated ok
• The engine tests still fail if the engine drops a URL — demonstrate it, e.g. by breaking something locally and showing them go red
• pytest tests/ -q green

Apologies to @XEDAB, who found the underlying bug, reported it precisely, and then got a wrong specification from me.