Re[2]: shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile [try 4]

2010-03-08 Thread Ilya Basin
>> 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.


-- 





Re[2]: shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile [try 4]

2010-03-06 Thread Ilya Basin
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).From 55dfdc06394130ca5c6ddac8d217e231f7501e44 Mon Sep 17 00:00:00 2001
From: Ilya Basin 
Date: Thu, 4 Mar 2010 21:48:58 +0300
Subject: shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile 
[try 5]
To: wine-patches 
Reply-To: wine-devel 

This should expose this bug:
http://bugs.winehq.org/show_bug.cgi?id=19385 ( the 'wine start'
launcher does not open MS Office documents that have spaces in their
path ). Although the title is misleading, many other tickets are
marked as duplicates of it.
It's not about spaces. It's about sometimes unneeded parsing of
SHELLEXECUTEINFO.lpFile, trying to extract arguments from it.

try 5:
replaced every win_skip() with broken()
try 4:
added these comments
try 3:
fixed test failures on 95/NT
new job results https://winetestbot.geldorp.nl/JobDetails.pl?Key=868
try 2:
added 4 todo_wine's to mark bad wine behavior
added GetProcAddress(hkernel32, "AttachConsole") to detect XP/2k3
removed special case when XP emulates 9x because can't do it without
using GetVersion().

---
 dlls/shell32/tests/shlexec.c |   83 ++
 1 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c
index 5d5db34..59ab61e 100644
--- a/dlls/shell32/tests/shlexec.c
+++ b/dlls/shell32/tests/shlexec.c
@@ -797,6 +797,10 @@ static const char* testfiles[]=
 "%s\\test file.sde",
 "%s\\test file.exe",
 "%s\\test2.exe",
+"%s\\simple.shlexec",
+"%s\\drawback_file.noassoc",
+"%s\\drawback_file.noassoc foo.shlexec",
+"%s\\drawback_nonexist.noassoc foo.shlexec",
 NULL
 };
 
@@ -852,6 +856,84 @@ static filename_tests_t noquotes_tests[]=
 {NULL, NULL, 0}
 };
 
+static void test_lpFile_parsed(void)
+{
+/* basename tmpdir */
+const char* shorttmpdir;
+
+const char *testfile;
+char fileA[MAX_PATH];
+
+int rc;
+int expected;
+
+GetTempPathA(sizeof(fileA), fileA);
+shorttmpdir = tmpdir + strlen(fileA);
+
+/* ensure tmpdir is in %TEMP%: GetTempPath() can succeed even if TEMP is 
undefined */
+SetEnvironmentVariableA("TEMP", fileA);
+
+#define TEST_LPFILE_PARSED_ok_(cond) ok((cond), "expected %s (%d), got %s 
(%d), lpFile: %s \n", \
+expected==33 ? "success" : "failure", expected, rc > 32 ? "success" : 
"failure", rc, fileA)
+
+#define TEST_LPFILE_PARSED_ok_condition() (rc==expected || (rc>32 && 
expected>32))
+#define TEST_LPFILE_PARSED_ok() 
TEST_LPFILE_PARSED_ok_(TEST_LPFILE_PARSED_ok_condition())
+
+/* existing "drawback_file.noassoc" 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);
+todo_wine { TEST_LPFILE_PARSED_ok(); }
+
+/* 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_(TEST_LPFILE_PARSED_ok_condition() || broken(rc == 
2) /* Win95/NT4 */ );
+
+/* error should be 2, not 31 */
+testfile = "\"%s\\drawback_file.noassoc\" foo.shlexec";
+expected = 2; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+TEST_LPFILE_PARSED_ok();
+
+/* ""command"" not works on wine (an