[PATCH 0/1] MR8353: amstream/tests: Add a few return value checks (Coverity).

[PATCH 1/1] amstream/tests: Add a few return value checks (Coverity).

[Nikolay Sivov]

From: Nikolay Sivov

---
 dlls/amstream/tests/amstream.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dlls/amstream/tests/amstream.c b/dlls/amstream/tests/amstream.c
index 7b0c8aec981..3dc0143d75b 100644
--- a/dlls/amstream/tests/amstream.c
+++ b/dlls/amstream/tests/amstream.c
@@ -8900,7 +8900,8 @@ static void test_ddrawstream_mem_allocator(void)
     hr = IDirectDrawMediaStream_SetFormat(ddraw_stream, &rgb555_format, NULL);
     ok(hr == S_OK, "Got hr %#lx.\n", hr);

-    IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw);
+    hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw);
+    ok(hr == S_OK, "Got hr %#lx.\n", hr);
     surface_desc.ddpfPixelFormat = rgb555_format.ddpfPixelFormat;
     hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &surface, NULL);
     ok(hr == S_OK, "got hr %#lx\n", hr);
@@ -9313,7 +9314,8 @@ static void test_ddrawstream_set_format_dynamic(void)
     hr = IDirectDrawMediaStream_SetFormat(ddraw_stream, &rgb8_format, NULL);
     ok(hr == S_OK, "Got hr %#lx.\n", hr);

-    IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw);
+    hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw);
+    ok(hr == S_OK, "Got hr %#lx.\n", hr);
     palette_entries[0].peRed = 0x12;
     palette_entries[1].peBlue = 0x34;
     palette_entries[2].peGreen = 0x56;

[Elizabeth Figura (＠zfigura)]

I won't reject this, but that was kind of an intentional choice, taking it as a given that GetDirectDraw() will succeed there, and avoiding an unnecessary line since it's not the point of the test. Is Coverity really warning about every unchecked HRESULT? That seems surprising; there's quite a lot in Wine.

[PATCH 0/1] MR8353: amstream/tests: Add a few return value checks (Coverity). - approved

[Elizabeth Figura (＠zfigura)]

This merge request was approved by Elizabeth Figura.

[Nikolay Sivov (＠nsivov)]

On Tue Jun 17 19:51:01 2025 +0000, Elizabeth Figura wrote:
> I won't reject this, but that was kind of an intentional choice, taking
> it as a given that GetDirectDraw() will succeed there, and avoiding an
> unnecessary line since it's not the point of the test. Is Coverity
> really warning about every unchecked HRESULT? That seems surprising;
> there's quite a lot in Wine.

Here it warns because there is 14 calls of the same function, but 2 of them are different. I think it makes sense, you either test for it everywhere or don't test it at all.

[Elizabeth Figura (＠zfigura)]

On Tue Jun 17 19:51:01 2025 +0000, Nikolay Sivov wrote:
> Here it warns because there is 14 calls of the same function, but 2 of
> them are different. I think it makes sense, you either test for it
> everywhere or don't test it at all.

I think it's excessive frankly. If you're trying to use some interface for example, you'd write QueryInterface() once, with a test that it's exposed, but once you've confirmed that in a formal test you might as well just write it without checking the return value from then on. That's common in quartz tests, and we might then write many tests that use that interface. This is a similar case.