[PATCH 1/2] winmm/tests: Add more mmioSeek tests.

Sagawa San,

it's nice to see somebody contribute to winmm!

+    return;
+}
That return is line noise.

+        hmmio = mmioOpen(NULL, &mmio, MMIO_READ);
+        if (hmmio == NULL) {
+            win_skip("mmioOpen error %u\n", mmio.wErrorRet);
+            return;
+        }
AFAIK mmioOpen has always been present, so there's no reason for a win_skip.
An error should be flagged in all cases, Wine or native.

Furthermore, the early return prevents execution of the other groups of tests.
Hence I suggest:
+        hmmio = mmioOpen(NULL, &mmio, MMIO_READ);
+       ok(hmmio != NULL, "mmioOpen error %u\n", mmio.wErrorRet);
+        if (hmmio) {
              mmioSeek ...
              mmioClose
          }

However, the 2nd and 3rd mmioOpen may fail if the test file could not be created,
so you may want a different treatment there.
Perhaps add a boolean result to create_test_file?
> [PATCH 2/2] winmm: Fix SEEK_END direction of mmio files without buffering.

It's always good in a patch submission to mention what bug it fixes.
Does yours fix bug 19566?  I seem to remember that there was an entry in
bugzilla about MMIO and seeking... Oh yes, you may look at bug 28982,
perhaps your patch fixes that too.

Regards,
 Jörg Höhle

[Akihiro Sagawa]

On Thu, 8 Nov 2012 11:12:04 +0100, Joerg-Cyril.Hoehle wrote:
Hi Jörg,

Thank you for your suggestions. I'll improve my  patches.
> > [PATCH 2/2] winmm: Fix SEEK_END direction of mmio files without buffering.
>
> It's always good in a patch submission to mention what bug it fixes.
> Does yours fix bug 19566?  I seem to remember that there was an entry in
> bugzilla about MMIO and seeking... Oh yes, you may look at bug 28982,
> perhaps your patch fixes that too.

Unfortunatelly, this patch does not fix bug 19566. While I was trying to
fix that bug, I noticed another issue in mmioSeek.
If mmioSeek patches are accepted, I'm sending some patches to fix bug
19566. The patch is now proposal stage [1].

[1] http://bugs.winehq.org/show_bug.cgi?id=19566#c12

Regards,
Akihiro Sagawa

[PATCH 1/2] winmm/tests: Add more mmioSeek tests. (try 2)

Hi again,

More comments about your tests #2:

+        DeleteFileA(test_file);
+[...]
+    has_test_file = create_test_file(test_file);
Why do you recreate an identical file between test group 2 and 3?

+    has_test_file = create_test_file(test_file);
+    ok(has_test_file, "failed to create test file\n");
This is redundant and thus superfluous because your
create_test_file ensures an ok() failure prior to every "return FALSE".
(perhaps add one ok check to CloseHandle?)

It's not always that easy to avoid correlated error messages,
I mean to try and produce only one error message per cause.
This will ease the job of people looking at failures on test.winehq.org.

+    char temp_path[MAX_PATH-14];
+    ret = GetTempPath(sizeof(temp_path), temp_path);
+    ret = GetTempFileName(temp_path, "mmio", 0, temp_file);
This is bogus. I believe you should use:
+    char temp_path[MAX_PATH];
+    ret = GetTempPath(sizeof(temp_path), temp_path);
+    ret = GetTempFileName(temp_path, "mmio", 0, temp_file);
and leave the -14 test to the implementation of GetTempFileName.

Regards,
    Jörg Höhle