Re: [PATCH] shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile (resend)

2010-02-12 Thread Paul Vriens 

On 02/11/2010 03:37 PM, Ilya Basin wrote:

Hi! 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. Why?
Or maybe some other part of wine relies on this behavior?
Tested on 98 se, 2k  xp.

...obvious mistakes become visible only after you hit the send
button. So I apologize beforehand.


Hi Ilya,

As Wine doesn't do the right thing (according to your tests), all 
failing Wine tests should be marked as such. We have a todo_wine 
construct for that.




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

diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c
index c673f0a..fb5e6b7 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,107 @@ 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;
+
+BOOL iswin9x = FALSE;
+BOOL isreallyNT = FALSE;
+
+/* detect win ver */
+{
+DWORD dwVersion = GetVersion();
+   DWORD dwWindowsMajorVersion =  (DWORD)(LOBYTE(LOWORD(dwVersion)));
+DWORD dwMinorVersion = (DWORD)(HIBYTE(LOWORD(dwVersion)));
+   DWORD dwBuild;
+
+   if (dwVersion  0x8000)  // Windows NT/2000, 
Whistler


No C++ comments please.


+   dwBuild = (DWORD)(HIWORD(dwVersion));
+   else if (dwWindowsMajorVersion  4)  // Win32s
+   dwBuild = (DWORD)(HIWORD(dwVersion)  ~0x8000);
+   else // Windows 95/98/Me
+   {
+   dwBuild =  0;
+iswin9x = TRUE;
+isreallyNT = NULL != GetModuleHandle(ntdll);
+   }
+}


We generally do not want any real version checking in the tests. Tests 
should decide based on behavior on what platform they are.



+
+GetTempPathA(sizeof(fileA), fileA);
+shorttmpdir = tmpdir + strlen(fileA);
+
+/* ensure tmpdir is in %TEMP% */
+SetEnvironmentVariableA(TEMP, fileA);


Why is this necessary?


+
+#define TEST_LPFILE_PARSED_OK_() (rc==expected || rc32  expected32)
+#define TEST_LPFILE_PARSED_OK() ok(TEST_LPFILE_PARSED_OK_(), expected %s (%d), got %s (%d), lpFile: %s \n, 
expected==33 ? success : failure, expected, rc  32 ? success : failure, rc, 
fileA)
+
+/* 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);
+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();
+
+/* 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 (and real win9x and w2k) */
+testfile = \\%s\\drawback_file.noassoc foo.shlexec\\;
+expected = iswin9x  !isreallyNT || dllver.dwMajorVersion= 5 ? 2 : 33; 
sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+TEST_LPFILE_PARSED_OK();
+
+/* nonexisting drawback_nonexist.noassoc not prevents finding 
drawback_nonexist.noassoc foo.shlexec on wine */
+testfile = %s\\drawback_nonexist.noassoc foo.shlexec;
+expected = 33; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+TEST_LPFILE_PARSED_OK();
+
+/* is SEE_MASK_DOENVSUBST default flag? Should only be when XP emulates 9x 
*/
+{


Any particular reason for this indentation?


--
Cheers,

Paul.

Re: [PATCH] shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile (resend)

2010-02-12 Thread Paul Vriens 

On 02/12/2010 12:39 PM, Ilya Basin wrote:

Thanks for review. Not sending to wine-patches this time. New patch is
in the bottom. What's better, to attach a generated patch or to use it
as a message body?


Depends on your mail client I guess. I usually attach but there are 
others who inline.



PV  Why is this necessary?
/* ensure tmpdir is in %TEMP%: GetTempPath() can succeed even if TEMP is 
undefined */


But do your tests actually rely on %TEMP% being defined? Not having a 
TEMP (or TMP) will probably makes loads of tests fail and I doubt one 
has a valid config without those.


Also when you sent a newer patch that has changes you should mark it as 
'try x' instead of 'resend'. 'Resend' is used when you think the patch 
has been missed by AJ for example.


--
Cheers,

Paul.

Re[2]: [PATCH] shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile (resend)

2010-02-12 Thread Ilya Basin 
Thanks for review. Not sending to wine-patches this time. New patch is
in the bottom. What's better, to attach a generated patch or to use it
as a message body?

PV On 02/11/2010 03:37 PM, Ilya Basin wrote:
 Hi! 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. Why?
 Or maybe some other part of wine relies on this behavior?
 Tested on 98 se, 2k  xp.

PV Hi Ilya,

PV As Wine doesn't do the right thing (according to your tests), all 
PV failing Wine tests should be marked as such. We have a todo_wine 
PV construct for that.
added 4 todo_wine's

PV We generally do not want any real version checking in the tests. Tests
PV should decide based on behavior on what platform they are.
added GetProcAddress(hkernel32, AttachConsole) to detect XP/2k3
removed special case when XP emulates 9x because can't do it without
using GetVersion().

 +
 +GetTempPathA(sizeof(fileA), fileA);
 +shorttmpdir = tmpdir + strlen(fileA);
 +
 +/* ensure tmpdir is in %TEMP% */
 +SetEnvironmentVariableA(TEMP, fileA);

PV Why is this necessary?
/* ensure tmpdir is in %TEMP%: GetTempPath() can succeed even if TEMP is 
undefined */

 +/* is SEE_MASK_DOENVSUBST default flag? Should only be when XP emulates 
 9x */
 +{

PV Any particular reason for this indentation?
OK

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

diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c
index c673f0a..d575d22 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,88 @@ 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;
+
+HMODULE hkernel32 = GetModuleHandle(kernel32);
+BOOL isreallyXP2k3plus = NULL != GetProcAddress(hkernel32, 
AttachConsole);
+
+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_() (rc==expected || rc32  expected32)
+#define TEST_LPFILE_PARSED_OK() ok(TEST_LPFILE_PARSED_OK_(), expected %s 
(%d), got %s (%d), lpFile: %s \n, expected==33 ? success : failure, 
expected, rc  32 ? success : failure, rc, fileA)
+
+/* 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();
+
+/* 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 (and real win9x and w2k) */
+if (isreallyXP2k3plus) {
+testfile = \\%s\\simple.shlexec\\;
+expected = 33; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+todo_wine { TEST_LPFILE_PARSED_OK(); }
+} else {
+win_skip(ShellExecute('\\command\\', ...) only works on XP/2k3 or 
newer\n);
+}
+
+/* nonexisting drawback_nonexist.noassoc not prevents finding 
drawback_nonexist.noassoc foo.shlexec on wine */
+testfile = %s\\drawback_nonexist.noassoc foo.shlexec;
+expected = 33; sprintf(fileA, testfile, tmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+TEST_LPFILE_PARSED_OK();
+
+/* is SEE_MASK_DOENVSUBST default flag? Should only be when XP emulates 9x 
(XP bug or real 95 or ME behavior ?) */
+testfile = %%TEMP%%\\%s\\simple.shlexec;
+expected = 2; sprintf(fileA, testfile, shorttmpdir);
+rc=shell_execute(NULL, fileA, NULL, NULL);
+todo_wine { TEST_LPFILE_PARSED_OK(); }
+
+/*

Re: Re[2]: [PATCH] shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile (resend)

2010-02-12 Thread James Mckenzie 
Ilya Basin basini...@gmail.com wrote at Feb 12, 2010 4:39 AM about Re[2]: 
[PATCH] shell32/tests: add tests for the parser of  SHELLEXECUTEINFO.lpFile 
(resend)

Thanks for review. Not sending to wine-patches this time. New patch is
in the bottom. What's better, to attach a generated patch or to use it
as a message body?

Generated patch, if at all possible.  That way we can use git apply to install.

James McKenzie