[PATCH v2] shlwapi/tests: Document where SHFormatDateTimeA() crashes for NULL pointers.
Knowing which Windows versions have been confirmed to crash will make it easier to know whether that information is potentially obsolete if the Windows behavior changes. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Indented. The if (0) is likely to stay there for a good while so it makes sense to indent the corresponding test. --- dlls/shlwapi/tests/ordinal.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/dlls/shlwapi/tests/ordinal.c b/dlls/shlwapi/tests/ordinal.c index bf43d35b1c2..8a294ec1751 100644 --- a/dlls/shlwapi/tests/ordinal.c +++ b/dlls/shlwapi/tests/ordinal.c @@ -1674,11 +1674,8 @@ static void test_SHFormatDateTimeA(void) DWORD flags; INT ret; -if (0) -{ - /* crashes on native */ - pSHFormatDateTimeA(NULL, NULL, NULL, 0); -} + if (0) /* crashes on Windows XP to Windows 10 20H2 */ + pSHFormatDateTimeA(NULL, NULL, NULL, 0); GetLocalTime(&st); SystemTimeToFileTime(&st, &filetime); -- 2.30.2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=96996 Your paranoid android. === debiant2 (32 bit Hindi:India report) === shlwapi: ordinal.c:1724: Test failed: expected ( ), got (02:32) ordinal.c:1731: Test failed: expected ( ), got (02:32:07) ordinal.c:1739: Test failed: expected ( ), got (02:32:07) ordinal.c:1746: Test failed: expected ( ), got (01-09-2021) ordinal.c:1753: Test failed: expected ( ), got (01 ??????? 2021) ordinal.c:1761: Test failed: expected ( ), got (01 ??????? 2021) ordinal.c:1769: Test failed: expected (F), got (2) for time part ordinal.c:1783: Test failed: expected (F), got (7) for time part ordinal.c:1801: Test failed: expected ( F), got (01-09-2021 02:32) ordinal.c:1812: Test failed: expected ( F F), got (01-09-2021 02:32:07)
On Wed, Sep 01, 2021 at 09:25:56AM +0200, Francois Gouget wrote:
Knowing which Windows versions have been confirmed to crash will make it easier to know whether that information is potentially obsolete if the Windows behavior changes.
Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Indented.
The if (0) is likely to stay there for a good while so it makes sense to indent the corresponding test. --- dlls/shlwapi/tests/ordinal.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/dlls/shlwapi/tests/ordinal.c b/dlls/shlwapi/tests/ordinal.c index bf43d35b1c2..8a294ec1751 100644 --- a/dlls/shlwapi/tests/ordinal.c +++ b/dlls/shlwapi/tests/ordinal.c @@ -1674,11 +1674,8 @@ static void test_SHFormatDateTimeA(void) DWORD flags; INT ret;
-if (0) -{ - /* crashes on native */ - pSHFormatDateTimeA(NULL, NULL, NULL, 0); -} + if (0) /* crashes on Windows XP to Windows 10 20H2 */ + pSHFormatDateTimeA(NULL, NULL, NULL, 0);
I don't see the point of this sort of thing. Why do we care that Windows crashes? No sane app is ever going to rely on this. Documenting which Windows versions crash seems even more of a waste of time; are we really going to update this after every new Windows release? Huw.
On Wed, 1 Sep 2021, Huw Davies wrote: [...]
I don't see the point of this sort of thing. Why do we care that Windows crashes? No sane app is ever going to rely on this.
I disagree with your statement "No sane app is ever going to rely on this". For instance GdipGetFamilyName() used to crash when given a NULL output buffer (ad663360fae6). But that changed with Windows 7; 12 years ago. So I'd argue that it's now reasonable for developers to assume GdipGetFamilyName() does not crash in that case. The test says that SHFormatDateTimeA(NULL...) "crashes on native". But _if_ that only describes the behavior of "Windows Vista or older" I'd argue that it's likewise reasonable for current applications to rely on the Windows 7+ behavior. Now it turns out SHFormatDateTimeA() crashes even in the latest Windows versions so I'm just documenting that. Also what is really tested is whether a NULL input pointer is allowed rather than a NULL output buffer. That may be less relevant, and could justify removing the test entirely.
Documenting which Windows versions crash seems even more of a waste of time;
I'm not going to go through all the test in Wine to update them if that's what you're afraid of. But when I try to understand how an API works in order to fix a test (here a shlwapi:ordinal failure in Japanese) and I see such a badly documented test I'll certainly check it out. Also any patch that contains an "if (0) /* crashes */" should have better documentation before it gets applied.
are we really going to update this after every new Windows release?
I'm fine with removing the test if you prefer that. But if it's going to stay it should have some indication of which Windows version crashed. -- Francois Gouget <fgouget(a)codeweavers.com>
On Wed, Sep 01, 2021 at 11:04:26AM +0200, Francois Gouget wrote:
On Wed, 1 Sep 2021, Huw Davies wrote: [...]
I don't see the point of this sort of thing. Why do we care that Windows crashes? No sane app is ever going to rely on this.
I disagree with your statement "No sane app is ever going to rely on this".
For instance GdipGetFamilyName() used to crash when given a NULL output buffer (ad663360fae6). But that changed with Windows 7; 12 years ago. So I'd argue that it's now reasonable for developers to assume GdipGetFamilyName() does not crash in that case.
Sure, there may be specific apps that call specific APIs that'll need investigation, tests, and fixing, but these will be the exception.
Also any patch that contains an "if (0) /* crashes */" should have better documentation before it gets applied.
Or better yet, not applied at all IMHO.
I'm fine with removing the test if you prefer that.
Works for me. Huw.
Hey folks, not to derail you, but this triggers me as a former contributor to winetest... On 2021-09-01 12:24, Huw Davies wrote:
On Wed, Sep 01, 2021 at 11:04:26AM +0200, Francois Gouget wrote:
On Wed, 1 Sep 2021, Huw Davies wrote: [...]
I don't see the point of this sort of thing. Why do we care that Windows crashes? No sane app is ever going to rely on this. ... Also any patch that contains an "if (0) /* crashes */" should have better documentation before it gets applied.
Or better yet, not applied at all IMHO.
I'm fine with removing the test if you prefer that.
Works for me.
If you remove it, a guy working on some old app (that misbehaves using Wine) will possibly try to add it back. Because one will not know the API crashes on newer Windows. But one will know it works in original setup eg. on Win 2000 + IE 5.01. My idea would be to leave two ranges in the comment, something like: /* Crashes on: every tested version */ /* Tested on: Windows XP to Windows 10 20H2 */ And leave it that. Whoever resumes investigation, one could update the ranges. This would cost the suite nothing, IMO. Especially given that there still are checks in the suite that dates back to even Win98/WinME. Which could carry some real cost in the terms of precision. My 2€¢, S. PS. Nice to see the ML so active:)
On Wed, 1 Sep 2021, Saulius Krasuckas wrote: [...]
If you remove it, a guy working on some old app (that misbehaves using Wine) will possibly try to add it back. Because one will not know the API crashes on newer Windows.
It's not clear that there ever was a Windows version where the API did not crash when given a NULL input pointer. Note that the NULL pointer being tested is an _input_ pointer (though that's not entirely obvious from the call). There is little the API can do without that single input. -- Francois Gouget <fgouget(a)codeweavers.com>
On 2021-09-02 11:21, Francois Gouget wrote:
On Wed, 1 Sep 2021, Saulius Krasuckas wrote: [...]
If you remove it, a guy working on some old app (that misbehaves using Wine) will possibly try to add it back. Because one will not know the API crashes on newer Windows.
It's not clear that there ever was a Windows version where the API did not crash when given a NULL input pointer.
That's why I call for saving the documented state (for possible research in future).
Note that the NULL pointer being tested is an _input_ pointer (though that's not entirely obvious from the call). There is little the API can do without that single input.
I agree. But for example when I tried to extend the LZOpenFile[AW] tests, I went for testing all combinations of wrong arguments I could imagine (still unfinished). That's because this way I could know more about the internal flow/branching of the black-boxed code. I mean, if some questions arises about when that input pointer is handled (before other argument validation or maybe after that, or maybe at the end), the crash (and maybe additional crashes) would give some insights about the original algorithm. Especially if the function works in some old native installation (like I made up before). Which we cannot test directly anymore of course, since Win2000 is already retired from the testing infrastructure. S.
On Thu, Sep 02, 2021 at 11:44:36AM +0300, Saulius Krasuckas wrote:
I agree. But for example when I tried to extend the LZOpenFile[AW] tests, I went for testing all combinations of wrong arguments I could imagine (still unfinished). That's because this way I could know more about the internal flow/branching of the black-boxed code.
I mean, if some questions arises about when that input pointer is handled (before other argument validation or maybe after that, or maybe at the end), the crash (and maybe additional crashes) would give some insights about the original algorithm.
Which is exactly why you should *not* write these sort of tests. We're aiming to match the functionality of the API without copying its implementation. We don't want to know about its internal branching structure. Of course if a particular app depends on Windows returning a specific error code then thats the time to add a test for it. Huw.
Finding out that a particular app breaks because of wrong error code might be taking hours quite often, days in the bad case. I was under impression accurately testing the error codes is a common Wine practice, unless it is too complicated. I think adding NULL parameters test is something we used to have in most of functions tests (not all the possible combinations of nulls of course). So if while adding the test we found that some null causes crash, why should we spare a line or two to reflect this with a comment and leave the reader to guess whether it was not tested ir crashing on some Windows versions?
On 2 Sep 2021, at 12:13, Huw Davies <huw(a)codeweavers.com> wrote:
Of course if a particular app depends on Windows returning a specific error code then thats the time to add a test for it.
Huw.
* On 2021-09-02 12:13, Huw Davies wrote:
On Thu, Sep 02, 2021 at 11:44:36AM +0300, Saulius Krasuckas wrote:
I mean, if some questions arises about when that input pointer is handled (before other argument validation or maybe after that, or maybe at the end), the crash (and maybe additional crashes) would give some insights about the original algorithm.
Which is exactly why you should *not* write these sort of tests.
Huw, I disagree that there can be a wrong sort of tests in black-box testing. Really don't see this.
We're aiming to match the functionality of the API without copying its implementation. We don't want to know about its internal branching structure.
And I aim at the functionality too: not the implementation, but the exactness of the behavior. IMO, it would be quite difficult to always ensure the project does the same function in the very different way. Difficult to prove and unnecessary. Because this is a black-box. Do I get this wrong? Plus, in case a developer starts being afraid that one or another test would reveal some unavoidable details of the original function/algorithm, one could quickly get lost in the doubt and start dropping every second ok-check in the area. Which would be a disaster for a black-box testing. I don't think these two things can be easily decoupled (even if the original implementation goes open source, even with the compatible license) or is it necessary at all. Just a PoV.
Of course if a particular app depends on Windows returning a specific error code then thats the time to add a test for it.
OK. And leaving the disabled test code only helps to do that (for everyone in doubt). I still saw no harm in that code. S. PS. As the removal patch already got accepted, I hope no one minds my response. :)
participants (5)
-
Francois Gouget -
Huw Davies -
Marvin -
Paul Gofman -
Saulius Krasuckas