Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests
Piotr Caban <piotr(a)codeweavers.com> wrote:
+ sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); + memset(&startup, 0, sizeof(startup)); + startup.cb = sizeof(startup); + CreateProcessA(path, cmd, NULL, NULL, TRUE, + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, + NULL, NULL, &startup, &proc); + winetest_wait_child_process(proc.hProcess);
Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have. -- Dmitry.
On 07/28/16 13:08, Dmitry Timoshkov wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
+ sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); + memset(&startup, 0, sizeof(startup)); + startup.cb = sizeof(startup); + CreateProcessA(path, cmd, NULL, NULL, TRUE, + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, + NULL, NULL, &startup, &proc); + winetest_wait_child_process(proc.hProcess);
Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have.
CreateProcess will fail in case of '\r' or '\n' characters.
Piotr Caban <piotr(a)codeweavers.com> wrote:
+ sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); + memset(&startup, 0, sizeof(startup)); + startup.cb = sizeof(startup); + CreateProcessA(path, cmd, NULL, NULL, TRUE, + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, + NULL, NULL, &startup, &proc); + winetest_wait_child_process(proc.hProcess);
Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have.
CreateProcess will fail in case of '\r' or '\n' characters.
In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you? -- Dmitry.
On 07/28/16 13:33, Dmitry Timoshkov wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
+ sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); + memset(&startup, 0, sizeof(startup)); + startup.cb = sizeof(startup); + CreateProcessA(path, cmd, NULL, NULL, TRUE, + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, + NULL, NULL, &startup, &proc); + winetest_wait_child_process(proc.hProcess);
Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have.
CreateProcess will fail in case of '\r' or '\n' characters.
In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you?
I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).
Piotr Caban <piotr(a)codeweavers.com> wrote:
+ sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); + memset(&startup, 0, sizeof(startup)); + startup.cb = sizeof(startup); + CreateProcessA(path, cmd, NULL, NULL, TRUE, + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, + NULL, NULL, &startup, &proc); + winetest_wait_child_process(proc.hProcess);
Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have.
CreateProcess will fail in case of '\r' or '\n' characters.
In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you?
I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).
Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. -- Dmitry.
On 07/28/16 15:27, Dmitry Timoshkov wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
+ sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); + memset(&startup, 0, sizeof(startup)); + startup.cb = sizeof(startup); + CreateProcessA(path, cmd, NULL, NULL, TRUE, + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, + NULL, NULL, &startup, &proc); + winetest_wait_child_process(proc.hProcess);
Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have.
CreateProcess will fail in case of '\r' or '\n' characters.
In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you?
I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).
Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. '\n' behaves the same as '\r' according to my test.
I think I have already explained you while '\r' case can't be tested in reasonable way.
Piotr Caban <piotr(a)codeweavers.com> wrote:
> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); > + memset(&startup, 0, sizeof(startup)); > + startup.cb = sizeof(startup); > + CreateProcessA(path, cmd, NULL, NULL, TRUE, > + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, > + NULL, NULL, &startup, &proc); > + winetest_wait_child_process(proc.hProcess);
Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have.
CreateProcess will fail in case of '\r' or '\n' characters.
In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you?
I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).
Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. '\n' behaves the same as '\r' according to my test.
I think I have already explained you while '\r' case can't be tested in reasonable way.
I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't fail, what you need to test is the resulting command line that the child process receives. It's clear and simple. -- Dmitry.
On 07/28/16 15:43, Dmitry Timoshkov wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >> + memset(&startup, 0, sizeof(startup)); >> + startup.cb = sizeof(startup); >> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >> + NULL, NULL, &startup, &proc); >> + winetest_wait_child_process(proc.hProcess); > > Thanks for adding the tests. It shouldn't be too hard to also test other > white space characters, at least testing '\r' and '\n' would be nice to > have. > CreateProcess will fail in case of '\r' or '\n' characters.
In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you?
I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).
Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. '\n' behaves the same as '\r' according to my test.
I think I have already explained you while '\r' case can't be tested in reasonable way.
I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't fail, what you need to test is the resulting command line that the child process receives. It's clear and simple.
I've already wrote you that '\r' is treated as argument - because of that it can't be added before "misc" argument (the test executable needs first argument to be "misc"). The function _get_narrow_winmain_command_line only interprets part of the command line before first argument.
Piotr Caban <piotr(a)codeweavers.com> wrote:
>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >>> + memset(&startup, 0, sizeof(startup)); >>> + startup.cb = sizeof(startup); >>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >>> + NULL, NULL, &startup, &proc); >>> + winetest_wait_child_process(proc.hProcess); >> >> Thanks for adding the tests. It shouldn't be too hard to also test other >> white space characters, at least testing '\r' and '\n' would be nice to >> have. >> > CreateProcess will fail in case of '\r' or '\n' characters.
In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you?
I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).
Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. '\n' behaves the same as '\r' according to my test.
I think I have already explained you while '\r' case can't be tested in reasonable way.
I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't fail, what you need to test is the resulting command line that the child process receives. It's clear and simple.
I've already wrote you that '\r' is treated as argument - because of that it can't be added before "misc" argument (the test executable needs first argument to be "misc"). The function _get_narrow_winmain_command_line only interprets part of the command line before first argument.
How about adding '\r' after "misc" or at whatever place you think appropriate and simply test both the resulting command line and the result of narrow one? Just execute the process and test whatever it gets as the command line and the narrow one? What is so hard with that? -- Dmitry.
On 07/28/16 17:25, Dmitry Timoshkov wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >>>> + memset(&startup, 0, sizeof(startup)); >>>> + startup.cb = sizeof(startup); >>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >>>> + NULL, NULL, &startup, &proc); >>>> + winetest_wait_child_process(proc.hProcess); >>> >>> Thanks for adding the tests. It shouldn't be too hard to also test other >>> white space characters, at least testing '\r' and '\n' would be nice to >>> have. >>> >> CreateProcess will fail in case of '\r' or '\n' characters. > > In a simple test CreateProcess() works just fine here under Windows 7 when a > passed in command line contains '\r' and/or '\n', where does it fail for you? > I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).
Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. '\n' behaves the same as '\r' according to my test.
I think I have already explained you while '\r' case can't be tested in reasonable way.
I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't fail, what you need to test is the resulting command line that the child process receives. It's clear and simple.
I've already wrote you that '\r' is treated as argument - because of that it can't be added before "misc" argument (the test executable needs first argument to be "misc"). The function _get_narrow_winmain_command_line only interprets part of the command line before first argument.
How about adding '\r' after "misc" or at whatever place you think appropriate and simply test both the resulting command line and the result of narrow one? Just execute the process and test whatever it gets as the command line and the narrow one? What is so hard with that?
Such test will be useless. It's already shown that _get_narrow_winmain_command_line returns substring of GetCommandLineA. It also doesn't touch anything past the beginning of first argument.
Piotr Caban <piotr(a)codeweavers.com> wrote:
>>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >>>>> + memset(&startup, 0, sizeof(startup)); >>>>> + startup.cb = sizeof(startup); >>>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >>>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >>>>> + NULL, NULL, &startup, &proc); >>>>> + winetest_wait_child_process(proc.hProcess); >>>> >>>> Thanks for adding the tests. It shouldn't be too hard to also test other >>>> white space characters, at least testing '\r' and '\n' would be nice to >>>> have. >>>> >>> CreateProcess will fail in case of '\r' or '\n' characters. >> >> In a simple test CreateProcess() works just fine here under Windows 7 when a >> passed in command line contains '\r' and/or '\n', where does it fail for you? >> > I'll try to be a little more precise - '\r' is treated as another > argument so it can't be used before first argument (misc in this case).
Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. '\n' behaves the same as '\r' according to my test.
I think I have already explained you while '\r' case can't be tested in reasonable way.
I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't fail, what you need to test is the resulting command line that the child process receives. It's clear and simple.
I've already wrote you that '\r' is treated as argument - because of that it can't be added before "misc" argument (the test executable needs first argument to be "misc"). The function _get_narrow_winmain_command_line only interprets part of the command line before first argument.
How about adding '\r' after "misc" or at whatever place you think appropriate and simply test both the resulting command line and the result of narrow one? Just execute the process and test whatever it gets as the command line and the narrow one? What is so hard with that?
Such test will be useless. It's already shown that _get_narrow_winmain_command_line returns substring of GetCommandLineA. It also doesn't touch anything past the beginning of first argument.
There is no such a thing as a useless test IMHO, any test exercises the API and shows its result preventing possible regressions in future. -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Piotr Caban