On Fri, Apr 10, 2009 at 6:15 AM, Nicolas Le Cam niko.lecam@gmail.com wrote:
Try2: This time with the patch.
Tested on Win2k/WinXP. Marked two tests as todo_wine.
I was serious before. You can't change the old tests. Please add new tests for the exposed bugs. Same goes for patch 3/4.
2009/4/10, James Hawkins truiken@gmail.com:
On Fri, Apr 10, 2009 at 6:15 AM, Nicolas Le Cam niko.lecam@gmail.com wrote:
Try2: This time with the patch.
Tested on Win2k/WinXP. Marked two tests as todo_wine.
I was serious before. You can't change the old tests. Please add new tests for the exposed bugs. Same goes for patch 3/4.
-- James Hawkins
I will for test 4 but test 3 wasn't correct before. It was assuming drive was current one and that wasn't correct as revealed by my patch. This was only right if run on systemdrive and not from root drive dir.
Thanks for review.
On Fri, Apr 10, 2009 at 2:36 PM, Nicolas Le Cam niko.lecam@gmail.com wrote:
2009/4/10, James Hawkins truiken@gmail.com:
On Fri, Apr 10, 2009 at 6:15 AM, Nicolas Le Cam niko.lecam@gmail.com wrote:
Try2: This time with the patch.
Tested on Win2k/WinXP. Marked two tests as todo_wine.
I was serious before. You can't change the old tests. Please add new tests for the exposed bugs. Same goes for patch 3/4.
-- James Hawkins
I will for test 4 but test 3 wasn't correct before. It was assuming drive was current one and that wasn't correct as revealed by my patch. This was only right if run on systemdrive and not from root drive dir.
You're missing the point. Even in 3/4 you are adding a todo_wine where there was none before. By definition, you are changing what is being tested. The test was not incorrect, it just didn't cover all corner cases. Like I said before, I have no problem with you adding a new test to tickle this bug you've found, but in your attempts to 'fix' the test for this corner case, you cannot add a todo_wine here. This requires you to acquire a deeper understanding of what is being tested instead of blindly throwing new code at the test.
2009/4/11 James Hawkins truiken@gmail.com:
On Fri, Apr 10, 2009 at 2:36 PM, Nicolas Le Cam niko.lecam@gmail.com wrote:
2009/4/10, James Hawkins truiken@gmail.com:
On Fri, Apr 10, 2009 at 6:15 AM, Nicolas Le Cam niko.lecam@gmail.com wrote:
Try2: This time with the patch.
Tested on Win2k/WinXP. Marked two tests as todo_wine.
I was serious before. You can't change the old tests. Please add new tests for the exposed bugs. Same goes for patch 3/4.
-- James Hawkins
I will for test 4 but test 3 wasn't correct before. It was assuming drive was current one and that wasn't correct as revealed by my patch. This was only right if run on systemdrive and not from root drive dir.
You're missing the point. Even in 3/4 you are adding a todo_wine where there was none before. By definition, you are changing what is being tested. The test was not incorrect, it just didn't cover all corner cases. Like I said before, I have no problem with you adding a new test to tickle this bug you've found, but in your attempts to 'fix' the test for this corner case, you cannot add a todo_wine here. This requires you to acquire a deeper understanding of what is being tested instead of blindly throwing new code at the test.
-- James Hawkins
Let met explain :
Running test on wine in folder C:\test : works (expected "C:\test" got "C:\test") Running test on wine in folder C:\ : works (expected "C:" got "C:") Running test on wine in folder Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests : works (expected "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests" got "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests") Running test on Windows in folder C:\test : works (expected "C:\test" got "C:\test") Running test on Windows in folder C:\ : fails (expected "C:" got "") Running test on Windows in folder X:\Documents And Settings\nlecam\Desktop : fails (expected "X:\Documents And Settings\nlecam\Desktop" got "C:\Documents And Settings\nlecam\Desktop")
That's why I considered it was incorrect. It only worked on a corner case, and never failed on Windows because it was certainly launched from C:\something every time. The only thing I can do to make this test completely correct is to check if expected directory is on SystemDrive and isn't the root drive directory, in this case the todo_wine isn't needed, i.e. limit current test to its corner case, mark it todo_wine and add second possible result in all other cases.
2009/4/11 Nicolas Le Cam niko.lecam@gmail.com:
2009/4/11 James Hawkins truiken@gmail.com: Let met explain :
Running test on wine in folder C:\test : works (expected "C:\test" got "C:\test") Running test on wine in folder C:\ : works (expected "C:" got "C:") Running test on wine in folder Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests : works (expected "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests" got "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests") Running test on Windows in folder C:\test : works (expected "C:\test" got "C:\test") Running test on Windows in folder C:\ : fails (expected "C:" got "")
This looks to me like it would be dependent on the WAY you run the test.
Running test on Windows in folder X:\Documents And Settings\nlecam\Desktop : fails (expected "X:\Documents And Settings\nlecam\Desktop" got "C:\Documents And Settings\nlecam\Desktop")
This looks like X: is mapped to the same place as C:, and wine's path translation is picking C: first. If that's so, it would also depend on the way you run the test.
Of course, I could be astronomically wrong :D
2009/4/11 Ben Klein shacklein@gmail.com:
2009/4/11 Nicolas Le Cam niko.lecam@gmail.com:
2009/4/11 James Hawkins truiken@gmail.com: Let met explain :
Running test on wine in folder C:\test : works (expected "C:\test" got "C:\test") Running test on wine in folder C:\ : works (expected "C:" got "C:") Running test on wine in folder Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests : works (expected "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests" got "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests") Running test on Windows in folder C:\test : works (expected "C:\test" got "C:\test") Running test on Windows in folder C:\ : fails (expected "C:" got "")
This looks to me like it would be dependent on the WAY you run the test.
Running test on Windows in folder X:\Documents And Settings\nlecam\Desktop : fails (expected "X:\Documents And Settings\nlecam\Desktop" got "C:\Documents And Settings\nlecam\Desktop")
This looks like X: is mapped to the same place as C:, and wine's path translation is picking C: first. If that's so, it would also depend on the way you run the test.
Of course, I could be astronomically wrong :D
Hi Ben,
Tests were launched from cmd.exe, current directory was where executable resides.
Those results (blank and X: becomming C:) were from Windows not Wine. X: is a network drive.
I admit I should do more tests, at least : * launch test from a real drive other than C:\ * launch test from another directory than where executable resides. * install Windows on another drive than C:, with or without a C:\ drive, to see if it's really SystemDrive that msi takes or just the first drive it can find.
Nicolas Le Cam wrote:
2009/4/11 Ben Klein shacklein@gmail.com:
2009/4/11 Nicolas Le Cam niko.lecam@gmail.com:
2009/4/11 James Hawkins truiken@gmail.com: Let met explain :
Running test on wine in folder C:\test : works (expected "C:\test" got "C:\test") Running test on wine in folder C:\ : works (expected "C:" got "C:") Running test on wine in folder Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests : works (expected "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests" got "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests") Running test on Windows in folder C:\test : works (expected "C:\test" got "C:\test") Running test on Windows in folder C:\ : fails (expected "C:" got "")
This looks to me like it would be dependent on the WAY you run the test.
Running test on Windows in folder X:\Documents And Settings\nlecam\Desktop : fails (expected "X:\Documents And Settings\nlecam\Desktop" got "C:\Documents And Settings\nlecam\Desktop")
This looks like X: is mapped to the same place as C:, and wine's path translation is picking C: first. If that's so, it would also depend on the way you run the test.
Of course, I could be astronomically wrong :D
Hi Ben,
Tests were launched from cmd.exe, current directory was where executable resides.
Those results (blank and X: becomming C:) were from Windows not Wine. X: is a network drive.
I admit I should do more tests, at least :
- launch test from a real drive other than C:\
- launch test from another directory than where executable resides.
- install Windows on another drive than C:, with or without a C:\
drive, to see if it's really SystemDrive that msi takes or just the first drive it can find.
Nicolas:
You should also test the cases that are failing from within Wine as well. We do have the capability to map to a X: drive by mapping a network drive to this letter as well.
It appears that C:\ is not returning a proper value either from Windows (which would not surprise me) or that Wine is not functioning the same as Windows in this case. You cannot mark this 'todo_wine' the code has to be fixed or the test has to be fixed.
James McKenzie
2009/4/12 James McKenzie jjmckenzie51@earthlink.net:
Nicolas Le Cam wrote:
2009/4/11 Ben Klein shacklein@gmail.com:
2009/4/11 Nicolas Le Cam niko.lecam@gmail.com:
2009/4/11 James Hawkins truiken@gmail.com: Let met explain :
Running test on wine in folder C:\test : works (expected "C:\test" got "C:\test") Running test on wine in folder C:\ : works (expected "C:" got "C:") Running test on wine in folder Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests : works (expected "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests" got "Z:\home\nlecam\Projects\wine\wine-git\dlls\msi\tests") Running test on Windows in folder C:\test : works (expected "C:\test" got "C:\test") Running test on Windows in folder C:\ : fails (expected "C:" got "")
This looks to me like it would be dependent on the WAY you run the test.
Running test on Windows in folder X:\Documents And Settings\nlecam\Desktop : fails (expected "X:\Documents And Settings\nlecam\Desktop" got "C:\Documents And Settings\nlecam\Desktop")
This looks like X: is mapped to the same place as C:, and wine's path translation is picking C: first. If that's so, it would also depend on the way you run the test.
Of course, I could be astronomically wrong :D
Hi Ben,
Tests were launched from cmd.exe, current directory was where executable resides.
Those results (blank and X: becomming C:) were from Windows not Wine. X: is a network drive.
I admit I should do more tests, at least :
- launch test from a real drive other than C:\
- launch test from another directory than where executable resides.
- install Windows on another drive than C:, with or without a C:\
drive, to see if it's really SystemDrive that msi takes or just the first drive it can find.
Nicolas:
You should also test the cases that are failing from within Wine as well. We do have the capability to map to a X: drive by mapping a network drive to this letter as well.
It appears that C:\ is not returning a proper value either from Windows (which would not surprise me) or that Wine is not functioning the same as Windows in this case. You cannot mark this 'todo_wine' the code has to be fixed or the test has to be fixed.
James McKenzie
Ok I did more tests, basically msi is only wrong on root drive directories, test is wrong on everything except when run on a local drive where pathes don't exist on a drive before (alphabetically sorted).
On this special test, msi enumerates local drives (and only local drives) and returns the first path that matches what was passed, if nothing match it returns an empty string.
Attached all tests I did. I will try to update my patch to handle every corner cases.
James,
Here are updated patches for part 3 & 4 of my previous patch set. Tell me if you think I could submit them to wine-patches.
For patch 3, the only todo_wine is when launched from root drive dir as explained in my previous mail. For patch 4, I skip a test if test is executed on root drive dir, as it doesn't make sense to execute it in this case.
If needed I can remove the skip and change expected value to be first fixed drive when test is run on root drive dir. But this is already tested by patch 3 (when run on a root drive dir).
-
Ben Klein -
James Hawkins -
James McKenzie -
Nicolas Le Cam