Test a stream read that fails on the first attempt - #1490
Conversation
FailingStreamBuf serves 32 bytes before it throws, so Stream's constructor completes and the failure is raised later, while scanning. Stream's constructor ends in ReadAheadTo(0), so a buffer that throws immediately raises it from inside the constructor instead - a path nothing covered. It is also the path that leaked before m_pPrefetched became a unique_ptr: a destructor does not run for an object whose constructor threw. Restoring the raw pointer keeps every existing test passing and green, while this one reports 2048 bytes leaked under AddressSanitizer.
There was a problem hiding this comment.
🟢 Approval recommended
The change is a focused, correct test addition that increases coverage for an important constructor-failure path without altering production behavior.
Pull request overview
Adds an integration test to cover the YAML::Stream constructor failure path when the underlying std::streambuf throws on the first prefetch read, ensuring Load(std::istream&) consistently translates that failure into BadStream (and guarding against regressions in constructor-exception/leak scenarios).
Changes:
- Introduce
ImmediatelyFailingStreamBuf, astd::stringbufthat throws on its firstxsgetncall. - Add
RejectsInputStreamFailureOnFirstReadto verifyLoad(stream)throwsBadStreamwhen the first prefetch read fails.
File summaries
| File | Description |
|---|---|
| test/integration/load_node_test.cpp | Adds a new failing streambuf and a new test to exercise the “failure during Stream construction / first prefetch” path. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I am sorry, I have trouble understanding what this PR is for. To me it sounds you are raising an issue and this PR demonstrates it? |
Yes, the issue itself has been fixed on master, but your test is incomplete so that's my suggestion. |
FailingStreamBuf serves 32 bytes before it throws, so Stream's constructor completes and the failure is raised later, while scanning. Stream's constructor ends in ReadAheadTo(0), so a buffer that throws immediately raises it from inside the constructor instead - a path nothing covered.
It is also the path that leaked before m_pPrefetched became a unique_ptr: a destructor does not run for an object whose constructor threw. Restoring the raw pointer keeps every existing test passing and green, while this one reports 2048 bytes leaked under AddressSanitizer.