[Bug] Unit Test Failures on Win7

There seems to be a difference causing test failures when using Win7 shlwapi.dll 6.1.7100. These tests pass fine using shlwapi.dll 6.00.2900.5512

2>[ PASSED ] 124 tests.
2>[ FAILED ] 12 tests, listed below:
2>[ FAILED ] PathAppendTest.ParentBasedBasicPathAndBasicName
2>[ FAILED ] PathAppendTest.ParentBasedFullPathAndBasicName
2>[ FAILED ] PathAppendTest.ParentBasedBasicPathAndRootPath
2>[ FAILED ] PathAppendTest.ParentBasedFullPathAndRootPath
2>[ FAILED ] PathAppendTest.ParentOfTwoDeepPathBackOneParentAndBasicName
2>[ FAILED ] PathRemoveFileSpecTest.BasicPathNoFileDoubleMiddleSlashWithoutEndSlash
2>[ FAILED ] PathCanonicalizeTest.ParentOverflow
2>[ FAILED ] PathCanonicalizeTest.ParentParentOverflow
2>[ FAILED ] PathCanonicalizeTest.SameParentOverflow
2>[ FAILED ] PathCanonicalizeTest.SameParentParentOverflow
2>[ FAILED ] PathCanonicalizeTest.ParentSameOverflow
2>[ FAILED ] PathCanonicalizeTest.ParentSameParentOverflow
2>12 FAILED TESTS

Reported by Thell Fowler · October 8th, 2009 @ 03:33 PM

Activity

Thell Fowler
A msdn forum thread has been opened on this issue at:

http://social.msdn.microsoft.com/Forums/en-US/vcprerelease/thread/0...

October 8th, 2009 @ 03:33 PM
Thell Fowler
- Milestone set to v1.0.0-rc1
October 8th, 2009 @ 03:35 PM
Jocelyn Legault
- Title changed from Unit Test Failures on Win7 to [Bug] Unit Test Failures on Win7
October 8th, 2009 @ 10:26 PM
Thell Fowler
- Milestone changed from v1.0.0-rc1 to v1.0.0
October 16th, 2009 @ 06:51 PM
Thell Fowler
- Milestone changed from v1.0.0 to v1.0.0-rc2
October 16th, 2009 @ 06:58 PM
Dave Brotherstone
- State changed from new to open
For the PathRemoveFileSpec failure, that is simply an improvement on their algorithm:
They're now handling c:\abc\def in PathRemoveFileSpec more sensibly, to become c:\abc rather than c:\abc\

For PathAppend, this is mainly due to a very strange change in behaviour in Win7, to do with PathCanonicalize.
```
..\abc\def = abc\def
```
- explicitly NOT what the docs say, it should be \abc\def, but, following the logic that if it starts with ..\, the \ is removed, we accept that change and continue...
```
\..\abc\def = \abc\def
```
- ok, this is the same as the previous version
```
..\abc\..\def = \def
```
- Pardon? ..\abc\def = abc\def, but ..\abc\..\def = \def, what on earth?
- if we re-wrote the path to be ..\def, which would be the same directory (relatively), it would return 'def', so a .. in the middle causes that rule to break
```
..\abc\def\..\ghi = abc\ghi
```
- but alas, that isn't always the case!
Hence, this is the logic that must be mirrored:
```
..\abc\def           abc\def
..\abc\..\def        \def
..\abc\def\..\ghi    abc\ghi
```
The generic_string version of PathCanonicalize needs to be re-developed using a list of path-sections, and then it can process the path from the start instead of the end, which will make the algorithm significantly simpler, albeit less efficient.
However, as can be seen from this table, the logic of whether the leading backslash is present is very odd.
October 18th, 2009 @ 09:07 PM
Jocelyn Legault
..\abc..\def = \def That makes sense IMO

if ..\abc becomes abc and ..\abc\def = \abc\def, then [..\abc][..\def] =>
[..\def] and [..\def] => \def

October 18th, 2009 @ 09:31 PM
Dave Brotherstone
Logic identified, I think, as if the "root" is ever touched, ignoring any leading .., then a backslash is prepended to the output.

..\abc\..\def
1. ignore initial ..
2. abc, now in directory abc
3. .., now at root, prepend \ to output
4. def, now at \def.
5. output result - \def
Requesting a vote on this:
1. Copy the generic_string's into TCHARs, call the win32 function and copy back
  or
2. Reimplement and do the windows version check for the edge case
October 18th, 2009 @ 10:12 PM
Jocelyn Legault
My vote goes strongly in favor of 1 for the following reasons:
- We will be assured that we will have an identical behaviour as the original Win32 one, no matter which platform / compiler we use.
- This function is never used in tight loops where realtime performance is needed.
So, while it'd be nice to have a "native" std::string implementation, it
think the better solution is the simplest in this case.
October 18th, 2009 @ 11:10 PM
Dave Brotherstone
- State changed from open to needs_ack
- Assigned user changed from Dave Brotherstone to npp-community
(from [250a8d32fb4d613b02b5fca91826ef25bc663097]) Common.cpp: PathXXX functions on Windows 7 fix
- PathCanonicalize has been made to simply call the Win32 function, as the difference between Win7 and prior versions is too great
- PathRemoveFileSpec added some extra logic in for double backslashes in the middle of a path
- Additional debug output added to tests to aid in debugging tests if they fail
[#4 state:needs_ack responsible:npp-community]

Signed-off-by: Dave Brotherstone davegb@pobox.com
http://github.com/davegb3/npp-community/commit/250a8d32fb4d613b02b5...
October 20th, 2009 @ 07:52 PM
Thell Fowler
- State changed from needs_ack to acked
Ack. Will get on pu soon.

October 20th, 2009 @ 08:10 PM
npp-community
- State changed from acked to needs_ack
(from [2a4b25270681344fb1162d0b0a8452b61e429007]) Common.cpp: PathXXX functions on Windows 7 fix
- PathCanonicalize has been made to simply call the Win32 function, as the difference between Win7 and prior versions is too great
- PathRemoveFileSpec added some extra logic in for double backslashes in the middle of a path
- Additional debug output added to tests to aid in debugging tests if they fail
[#4 state:needs_ack responsible:npp-community]

Signed-off-by: Dave Brotherstone davegb@pobox.com
http://github.com/npp-community/npp-community/commit/2a4b2527068134...
October 21st, 2009 @ 05:19 PM
Thell Fowler
- State changed from needs_ack to proposed
(from [5abf9740311f688a2a1e3d5369615fe60f30955f]) Merge branch 'db/LH-4/PathFuncFix' into pu
- db/LH-4/PathFuncFix: Common.cpp: PathXXX functions on Windows 7 fix
[#4 state:proposed]

Signed-off-by: Thell Fowler git@tbfowler.name
http://github.com/npp-community/npp-community/commit/5abf9740311f68...
October 21st, 2009 @ 05:19 PM
Thell Fowler
Thanks Dave!

I can confirm Win7 now passes all tests, I haven't tried this build on XP yet to verify it still passes all tests.

October 21st, 2009 @ 05:37 PM
Thell Fowler
- State changed from proposed to candidate
(from [67d226aa8999e6cb2db9fc618912af20ca2d57d2]) Merge branch 'db/LH-4/fix-win7-path-handling' into next
- db/LH-4/fix-win7-path-handling: Common.cpp: PathXXX functions on Windows 7 fix
[#4 state:candidate]

Signed-off-by: Thell Fowler git@tbfowler.name
http://github.com/npp-community/npp-community/commit/67d226aa8999e6...
October 26th, 2009 @ 04:46 PM
Thell Fowler
- State changed from candidate to resolved
(from [71c6fddd0f5457ad1dcbf834914ccf52b7ac4c43]) Merge branch 'db/LH-4/fix-win7-path-handling'
- db/LH-4/fix-win7-path-handling: Common.cpp: PathXXX functions on Windows 7 fix
[#4 state:resolved]

Signed-off-by: Thell Fowler git@tbfowler.name
http://github.com/npp-community/npp-community/commit/71c6fddd0f5457...
November 29th, 2009 @ 06:34 AM

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.