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

[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<  0x80000000)              // 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 || rc>32&&  expected>32)
> - #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?