conftest stubs download_file on moon_cli only — the engine keeps... - #163
Conversation
| common: "Common", common_sub: "· both methods", | ||
| extractors: "Extractors", dl_streams: "DL streams", retries: "Retries", | ||
| rec16: "rec. 16", rec8: "rec. 8", rec8p: "rec. 8", | ||
| note_extractors: "datanodes is limited to Pages lanes regardless of this value", |
There was a problem hiding this comment.
#160 Only mentioned fixing the tests in conftest.py; why modify the frontend?
| <output id="outWorkers">16</output> | ||
| <input type="range" id="workers" min="2" max="32" step="1" value="16" data-rec="16"> | ||
| </label> | ||
| <p class="note" data-i18n="note_extractors">datanodes is limited to Pages lanes regardless of this value</p> |
| cleanup() | ||
|
|
||
|
|
||
| def test_engine_download_mode_uses_stubbed_download_file(browser_calls): |
There was a problem hiding this comment.
The download mode test here is a good addition — I forgot to add this test in my #162 (sorry).
Moved the download_file stub inside the shared host loop so moon_engine gets it too; added regression test.
fad9d41 to
d49e96b
Compare
|
@harshvardhan60792 — thank you for this, and I owe you a clear explanation rather than a quiet close. On the ordering. #160 was offered to @XEDAB on 8 August (they reported the underlying problem while working on #157). They claimed it here on 9 August at 06:11 with their verification work, and opened #162 at 06:43. This pull request was opened at 20:00 the same day — about thirteen hours after there was already an open pull request against that issue. #162 is now merged. That is not a criticism of you. An open PR is easy to miss, and the issue had no assignee because GitHub only accepts assignees who have commented on the issue itself — a limitation I have run into repeatedly and have not solved. But when two people fix the same thing, the one who claimed it first and opened first gets it, and I would apply that rule the other way round without hesitating. Now the part that matters: your regression test is not in #162, it is genuinely wanted, and I verified that myself before writing this. I checked out your branch, reverted only the Against 13.48s with the fix in place. That eleven-second difference is the bug: without the stub the engine reaches the real downloader and makes an actual network request to a fake URL. Your test does not just assert the fix is present — it makes the failure mode visible in the runtime. That is a better demonstration than the manual recipe I put in the issue, and #162 does not have it. So: would you open it again as just the test? Branch fresh from
That is a real contribution standing on its own, not a consolation prize: it is the thing that stops #160 coming back. Reference this pull request in the body and I will review it the same day. If you would rather not, that is completely fair and no hard feelings — say so and I will open it as an issue crediting you, so nobody re-does your analysis from scratch. Either way: you read the issue properly, wrote a test that fails for the right reason, and stated in your description that it does — which I was able to confirm exactly as written. That is a good first showing here. |
3 participants
Fixes #160
conftest stubbed download_file on moon_cli only, leaving moon_engine holding the real function; moved the patch inside the shared host loop so both front-ends get it, with a comment explaining why.
Testing: Added test_engine_download_mode_uses_stubbed_download_file, which fails without the fix and passes with it; also confirmed it turns red when the engine drops a success increment, per the issue's verification recipe. Ran the full suite (pytest tests/ -q): 51 passed.
Description
Type of change
Checklist
applied the equivalent change to
moon_cli.pydescription
Screenshots / logs (if applicable)