-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8327236: JFileChooser/8194044/FileSystemRootTest.java fails on Windows 11: root drive reported as false #27329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back tr! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@TejeshR13 The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
@@ -36,6 +36,10 @@ | |||
|
|||
public class FileSystemRootTest { | |||
public static void main(String[] args) throws Exception { | |||
if (System.getProperty("os.name").equalsIgnoreCase("Windows 11")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the same behavior applies to versions newer than 11, right? If that's the case, perhaps you could check whether getParentDirectory("C:\...\Documents")
returns C:\..\Desktop
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether the same behavior applies to newer versions is unknown. As of now it works for Windows 10 and doesn't work for Windows 11. That the behavior seen in Windows 11 where FileSysetemView.getParentDirectory(C:/..../Documents)
return "C:/..../Desktop". So this is an observations and I think this particular test cannot handle the scenario, which is why I'm skipping it for only Window 11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should assume it does apply to newer versions.
However that is moot, as I don't see much point in continuing to run on the old, nearly out of support Windows 10.
Additionally I strongly suspect that you'll find that Windows Server 2022 and definitely 2025 will have the same behavior and you aren't preventing the test running there.
And if this is fixed, the bug tracking the problem is gone. So you'd just have to file another one and we've artificially hidden the test failure.
So I think this PR should be withdrawn.
I don't think it's a product bug. It's Microsoft decision to move around folders in Windows shell namespace. The test expectation could be wrong for other versions of Windows, however, Windows 11 will soon be the only version supported. Perhaps, the test should be written differently. As far as I understand, the problem with JDK-8194044 was that the drives had displayed without their names. The test navigates the shell namespace to find the root and then verifies the name isn't empty. Disabling the test for Windows 11 renders the test useless. The regression where disks display no name could be re-introduced. The fix should modify the test logic so that it is able to navigate to the root of the drive. Alternatively, you can call |
The logic no longer applies to window 11 since getParentDirectory("C:\...\Documents") return "C:\..\Desktop". This will require thorough analysis and might call for a product fix. Hence bypassing this test for Windows 11 OS.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27329/head:pull/27329
$ git checkout pull/27329
Update a local copy of the PR:
$ git checkout pull/27329
$ git pull https://git.openjdk.org/jdk.git pull/27329/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27329
View PR using the GUI difftool:
$ git pr show -t 27329
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27329.diff
Using Webrev
Link to Webrev Comment