Did you write this code yourself? Or was this written by an LLM? What is this actually testing?

@bashonly
i bashonly, thank you for the feedback.

Yes, I used an LLM to assist with formatting and English, but the test logic and design are my own.

My goal here is to add unit tests for the error-handling paths in download_and_append_fragments, specifically how it behaves during fatal errors, when skipping fragments, and on DownloadError.

Did you write this code yourself? Or was this written by an LLM? What is this actually testing?

Hi! I wrote the tests myself as part of a software testing course where we're applying MC/DC and unit testing techniques to real-world code.

The goal of this test suite is to validate different control paths and exception-handling behaviors of the download_and_append_fragments method in FragmentFD.

Each test targets a specific condition:

• CT1: Basic successful flow — fragment is downloaded, read, decrypted, and appended.
• CT2: Simulates a failed read on a fragment that is non-fatal. The fragment should be skipped and not crash the process.
• CT3: Same read failure, but this time it's considered fatal by is_fatal(index). The method should abort and call report_error.
• CT4: Simulates a DownloadError being raised. Since it's fatal, the method should let the exception propagate.

Let me know if I should clarify or improve anything. Happy to make adjustments.

Hi, please from now on avoid force pushing after opening the PR as it makes difficult for us to review

Strikingly, this test suite is attempting to verify a method with quite a complex signature that has no docstring to specify it and has survived for 3 years in that state. I think that would merit discussion in class or in your project report.

If, in the spirit of test-driven development, the test suite effectively defines the method's parameters and behaviour, you could create a docstring for the method that expresses that definition. And if it doesn't, more tests are needed ...

Looking at a related method not covered here, download_and_append_fragments_multiple() has a minimal docstring but could also say (if it is so) that its kwargs are the keyword-only parameters of download_and_append_fragments().

Let me prefix this by saying that while we would want to test the internals eventually, this will have to be planned. Its also questionable how much of it should be unit tested and how much integration tested. I will get to a review after the maintainers have reached a consensus on that, however this might require restructuring the tests. Just as a heads-up.

Additionally, assignments like these put maintainers under pressure, because it guilt-trips them into merging and reviewing code only so that people do not fail their assignments. Considering this, it should be reevaluated if these assignments should be conducted on Open Source libraries as is.

At first glance, a lot of things are being mocked. How did you decide what to mock and why?

If we need to use this many mocks, then the effectiveness of unit testing is questionable. IMO an integration test would make more sense.

Also the comments and strings are not in English; these would have to be translated if they persist.

@dirkf: While your input is valued, I ask that you refrain from commenting on PRs that are labeled as core-triage: this indicates that the PR needs to be evaluated by yt-dlp core maintainers before anyone else (including the author) should put any more thought or effort into it.