Hi Ilya,
I think you're still confused by the prohibition against using GetVersion. Your changes follow the letter of that edict, but not the spirit:
+ HMODULE hkernel32 = GetModuleHandle("kernel32"); + BOOL isreallyXP_2k3_plus = NULL != GetProcAddress(hkernel32, "AttachConsole"); + BOOL isreally98_2k_plus = NULL != GetProcAddress(hkernel32, "GetLongPathNameA");
That is, you're looking for other markers of particular versions of Windows. That's not the reason for avoiding the use of GetVersion. We avoid GetVersion because version checks in general are fragile. Sometimes a behavior will change due to the installation of a service pack, for example, and the tests will begin to fail if they expect a particular behavior on a particular version of Windows.
What we prefer is for tests to check the behavior of the function being tested on every version of Windows where that's possible. In some cases, it's impossible, because the function doesn't exist on old versions of Windows, or because the function crashes when called in a particular way. In such cases, we try to avoid crashes, and use win_skip to indicate that certain tests couldn't be performed.
In other cases, some older versions behave in ways that aren't really sensible. We mark these as broken(), indicating that it's acceptable for Windows to behave this way, but not for Wine to do so. I think this is what you want, e.g. here:
+ /* quoted lpFile not works on real win95 and nt4 */ + if (isreally98_2k_plus) { + /* if quoted, existing "drawback_file.noassoc" not prevents finding "drawback_file.noassoc foo.shlexec" on wine */ + testfile = ""%s\drawback_file.noassoc foo.shlexec""; + expected = 33; sprintf(fileA, testfile, tmpdir); + rc=shell_execute(NULL, fileA, NULL, NULL); + TEST_LPFILE_PARSED_OK(); + } else { + win_skip("ShellExecute('"command"', ...) only works on 98/2k or newer\n"); + }
What you want to do instead is to call shell_execute in any case (unless doing so would crash.) You'll want to check rc on succeeding versions of Windows/Wine with ok, and mark it as broken elsewhere. E.g., rc = shell_execute(NULL, fileA, NULL, NULL); ok((rc==expected || rc>32 && expected>32) || broken(rc == whatever busted versions of Windows return), "expected ...");
Also, those TEST_LPFILE_PARSED_OK macros are ugly, and hard to fix if one place has a certain behavior, while another has different behavior. Just use ok directly wherever you're using the macro.
Thanks, --Juan
Hello. I'll wait for your comments for some time before trying to send "try 5".
JL> I think you're still confused by the prohibition against using JL> GetVersion. Your changes follow the letter of that edict, but not the JL> spirit: lol, it's almost the same thing US customs inspector told a person I know, when she started to travel too often.
JL> What you want to do instead is to call shell_execute in any case JL> (unless doing so would crash.) You'll want to check rc on succeeding JL> versions of Windows/Wine with ok, and mark it as broken elsewhere. JL> E.g., JL> rc = shell_execute(NULL, fileA, NULL, NULL); JL> ok((rc==expected || rc>32 && expected>32) || broken(rc == whatever JL> busted versions of Windows return), "expected ..."); Thanks, I'll try to follow your recommendation. However, the drawback of using broken() is when running on wine, an .exe compiled with visual studio won't show test failures. Only "make test" in tests directory will notice these failures.
JL> Also, those TEST_LPFILE_PARSED_OK macros are ugly, and hard to fix if JL> one place has a certain behavior, while another has different JL> behavior. Just use ok directly wherever you're using the macro. Not convinced. What you say is good for reading, but not editing. If someone wants to change behavior , the other person defined in a macro, but only in one place, he can jast as simple copy-paste the definition and edit it. In any case, it's a free country (c).
On 03/06/2010 07:00 PM, Ilya Basin wrote:
JL> Also, those TEST_LPFILE_PARSED_OK macros are ugly, and hard to fix if JL> one place has a certain behavior, while another has different JL> behavior. Just use ok directly wherever you're using the macro. Not convinced. What you say is good for reading, but not editing. If someone wants to change behavior , the other person defined in a macro, but only in one place, he can jast as simple copy-paste the definition and edit it. In any case, it's a free country (c).
I must agree with Juan here.
Apart from that, the macro TEST_LPFILE_PARSED_ok_condition is strange in itself as it checks for 'expected' and this is something you always set yourself.
I agree with you that it's a free country and stuff like this will always be up for debate but in the end it's AJ that commits the patches and he has expressed several times not being to fond about too much macros.
JL> Also, those TEST_LPFILE_PARSED_OK macros are ugly, and hard to fix if JL> one place has a certain behavior, while another has different JL> behavior. Just use ok directly wherever you're using the macro. Not convinced. What you say is good for reading, but not editing. If someone wants to change behavior , the other person defined in a macro, but only in one place, he can jast as simple copy-paste the definition and edit it. In any case, it's a free country (c).
PV> I must agree with Juan here.
PV> Apart from that, the macro TEST_LPFILE_PARSED_ok_condition is strange in PV> itself as it checks for 'expected' and this is something you always set PV> yourself.
PV> I agree with you that it's a free country and stuff like this will PV> always be up for debate but in the end it's AJ that commits the patches PV> and he has expressed several times not being to fond about too much macros.
If it's well known, you had to tell me about macros at the beginning. Sent try 6.
On 03/08/2010 05:40 PM, Ilya Basin wrote:
JL> Also, those TEST_LPFILE_PARSED_OK macros are ugly, and hard to fix if JL> one place has a certain behavior, while another has different JL> behavior. Just use ok directly wherever you're using the macro. Not convinced. What you say is good for reading, but not editing. If someone wants to change behavior , the other person defined in a macro, but only in one place, he can jast as simple copy-paste the definition and edit it. In any case, it's a free country (c).
PV> I must agree with Juan here.
PV> Apart from that, the macro TEST_LPFILE_PARSED_ok_condition is strange in PV> itself as it checks for 'expected' and this is something you always set PV> yourself.
PV> I agree with you that it's a free country and stuff like this will PV> always be up for debate but in the end it's AJ that commits the patches PV> and he has expressed several times not being to fond about too much macros.
If it's well known, you had to tell me about macros at the beginning.
This is not a general rule, just something I noticed lately.
Sent try 6.
Thanks for your dedication.
-
Ilya Basin -
Juan Lang -
Paul Vriens