On 11/07/2010 04:06 PM, David Hedberg wrote:
- ok(hr == S_OK, "got (0x%08x)\n", hr);
- ok(hr == S_OK || hr == E_FAIL /* Win7 */, "got (0x%08x)\n", hr);
This can't be correct. It either works or it fails. Can't be both at the same time. You should look into why it's failing on Win7 and correct the test so it succeeds.
Vitaliy
On Mon, Nov 8, 2010 at 01:22, Vitaliy Margolen wine-devel@kievinfo.com wrote:
- ok(hr == S_OK, "got (0x%08x)\n", hr);
- ok(hr == S_OK || hr == E_FAIL /* Win7 */, "got (0x%08x)\n", hr);
This can't be correct. It either works or it fails. Can't be both at the same time. You should look into why it's failing on Win7 and correct the test so it succeeds.
I guess it makes the test a bit less useful for catching any errors, but reading between the lines at msdn makes me suspect that passing NULL for the pidl here simply doesn't work under Windows 7. I just tried the same thing on a IShellFolderView created from the windows directory and it gave the same result (still the default shellview I guess).
Assuming for the moment that this is indeed the only result you'd ever get, should I find a way to skip it on windows 7 or mark one of the results as broken? I don't quite see either alternative as very helpful in this case, but I might be wrong.
David
On 11/7/10 6:41 PM, David Hedberg wrote:
On Mon, Nov 8, 2010 at 01:22, Vitaliy Margolenwine-devel@kievinfo.com wrote:
- ok(hr == S_OK, "got (0x%08x)\n", hr);
- ok(hr == S_OK || hr == E_FAIL /* Win7 */, "got (0x%08x)\n", hr);
This can't be correct. It either works or it fails. Can't be both at the same time. You should look into why it's failing on Win7 and correct the test so it succeeds.
I guess it makes the test a bit less useful for catching any errors, but reading between the lines at msdn makes me suspect that passing NULL for the pidl here simply doesn't work under Windows 7. I just tried the same thing on a IShellFolderView created from the windows directory and it gave the same result (still the default shellview I guess).
Assuming for the moment that this is indeed the only result you'd ever get, should I find a way to skip it on windows 7 or mark one of the results as broken? I don't quite see either alternative as very helpful in this case, but I might be wrong.
Just my .02 USD (in other words, 2 cents) as a long term QA person, not as a Wine Developer. A test should not fail unless that is the desired result. However, marking an existing test as having new failure condition is not correct. Now, it may be true that the test passes up to and including Windows2008 but now fails on Windows7. Thus a second test case needs to be developed that is only for Windows7 and the remaining test skipped for Windows7. Something like what we do for Unicode tests for Windows9x/ME. This is not the first test that a failure will be a pass on Windows7 where a failure is not desired on prior versions of Windows.
Unfortunately, I don't know what to do at this point, but maybe there is a new function that exists only in Windows7 for shell32 that can be used for the test.
James McKenzie
On 8 November 2010 11:49, James McKenzie jjmckenzie51@earthlink.net wrote:
Thus a second test case needs to be developed that is only for Windows7 and the remaining test skipped for Windows7. Something like what we do for Unicode tests for Windows9x/ME.
Isn't the rule that the tests should only check windows versions before testing behaviour if that is what applications to a similar thing, otherwise the test isn't required?
On 8 November 2010 04:45, Austin Lund austin.lund@gmail.com wrote:
On 8 November 2010 11:49, James McKenzie jjmckenzie51@earthlink.net wrote:
Thus a second test case needs to be developed that is only for Windows7 and the remaining test skipped for Windows7. Something like what we do for Unicode tests for Windows9x/ME.
Isn't the rule that the tests should only check windows versions before testing behaviour if that is what applications to a similar thing, otherwise the test isn't required?
The rule is not to check Windows version, but to infer the DLL version (e.g. by checking for methods only available in newer versions of Windows). That way, installing a new version of shlwapi.dll via Internet Explorer on older systems does not break those tests.
In this case, the older behaviour is deprecated, so at least needs a:
todo_wine ok(broken(hr == S_OK) /* <= Vista */ || hr == E_FAIL /* Win7 */, ...)
or the test case needs to be removed (as different versions of Windows have conflicting behaviour, so applications cannot rely on either).
If an application requires this to work the test should be:
ok(hr == S_OK /* Required by SomeApp */ || broken(hr == E_FAIL) /* Win7 */, ...)
- Reece
On 8 November 2010 17:32, Reece Dunn msclrhd@googlemail.com wrote:
On 8 November 2010 04:45, Austin Lund austin.lund@gmail.com wrote:
On 8 November 2010 11:49, James McKenzie jjmckenzie51@earthlink.net wrote:
Thus a second test case needs to be developed that is only for Windows7 and the remaining test skipped for Windows7. Something like what we do for Unicode tests for Windows9x/ME.
Isn't the rule that the tests should only check windows versions before testing behaviour if that is what applications to a similar thing, otherwise the test isn't required?
The rule is not to check Windows version, but to infer the DLL version (e.g. by checking for methods only available in newer versions of Windows). That way, installing a new version of shlwapi.dll via Internet Explorer on older systems does not break those tests.
That seems like a test which is necessarily true, but not sufficient. If the DLL ABI does not change the tests results might. In the spirit of the rule described on the wiki (and quoted above), it would seem highly unusual for a program to test for the presence or absence of an unrelated function before depending on some result.
The use of broken() has taken me a while to understand. I believe it is better understood as not "broken" in particular, but a behaviour that wine should not reproduce but sometimes windows might (or does).
-
Austin Lund -
David Hedberg -
James McKenzie -
Reece Dunn -
Vitaliy Margolen