Re: [patch] riched20 conformance test

[Mike McCormack]

[e-mail] wrote:
> I have started writing a conformance test for the rich edit control 
> (riched20). The purpose is for students in UCLA's CS 130 Software 
> Engineering course to first extend this conformance test to test 
> conformance to unimplemented features, and then later implement their 
> chosen features.
>
> You can find the patch here:
> http://www.seas.ucla.edu/~kho/wine/patches/riched20_test_20060119.patch
>
> Any comments would be much appreciated!

Hi Thomas,

If you're submitting the patch, make sure to send it to 
[e-mail]. as that's the only place that patches will be 
picked up from.

The test case looks good, just a few comments from me:

* You linked the test with "riched20.dll", so the following call should 
always succeed.  Since you're trying to write a test, maybe that's what 
you want.  If you're trying to be safe incase there's a Windows platform 
that doesn't have RICHED20.DLL, it won't work.  Also, you probably want 
a FreeLibrary() at the end if you have a LoadLibrary.  Perhaps you just 
want GetModuleHandle() instead?

+  hmoduleRichEdit = LoadLibrary("RICHED20.DLL");

* in the "haystack" string, you can put quote on both sides of the 
string, and then indents... maybe looks nicer? eg.

static const char haystack[] = "Think of Wine as a compatibility layer "

"for running Windows programs. Wine does not require "
     "Microsoft Windows, as it is a "

* You've probably seen the SetLastError(0xdeadbeef) call in other tests. 
  The idea is to set the last error to a known value, and then test the 
value after the API call, comparing GetLastError() to a known value.  In 
this case you'd want to check that it's still "0xdeadbeef" because it 
shouldn't be set if CreateCompatibleDC succeeds. You don't even need to 
set it if you don't plan to test the return value of GetLastError().

+  SetLastError(0xdeadbeef);
+  hDCMem = CreateCompatibleDC(hDCWnd);
+  ok(hDCMem != NULL, "error: %d\n", (int) GetLastError());

* Calls with no parameters should be void to avoid triggering a warning 
with -Wstrict-prototypes.  See http://wiki.winehq.org/CompilerWarnings

+static void test_EM_FINDTEXT()

* better than commenting out a test, add "if(0)" in front of it, and 
then you won't get "function declared static but not used" warnings.

+  /*test_EM_EXLIMITTEXT();*/ /* too slow */

* you subclass the richedit window, maybe you should set the original 
window procedure back again before destroying the window?

+  ok(0 != SetWindowLongPtr(parent, GWLP_WNDPROC,
+                           (LONG_PTR) proc_EM_AUTOURLDETECT) ...

I'm not sure that comparing the screen bitmaps will work for all cases. 
  For example, if the size of the display, or the size of the font 
changes, then the size of the richedit window might also change...

+    screen_diff = screens_differ(before, after, 0, 0, 200, 50);

Finally, it's not 100% necessary, but it might help you get the patch 
accepted - you might consider cutting the patch up into two chunks, one 
with the essential Makefile stuff and a few tests you're pretty sure of, 
then add further tests later.  You don't have to have your patch 
accepted in one go, and dividing it into smaller parts (within reason) 
can help.

thanks,

Mike