Skip to content
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

Add tests for filenames with surrogate pairs on Windows #51789

Closed
AlttiRi opened this issue Feb 17, 2024 · 11 comments
Closed

Add tests for filenames with surrogate pairs on Windows #51789

AlttiRi opened this issue Feb 17, 2024 · 11 comments
Labels
feature request Issues that request new features to be added to Node.js. windows Issues and PRs related to the Windows platform.

Comments

@AlttiRi
Copy link

AlttiRi commented Feb 17, 2024

What is the problem this feature will solve?

That issue is still not fixed in LTS. There are no guarantees that the bug will not appear in Current again.

What is the feature you are proposing to solve the problem?

Just add tests for fs functions that will work with names contain surrogate pairs (emoji, but not only):

import fs from "node:fs/promises";

await fs.writeFile("🚀🔥🛸.txt", "");
await fs.lstat("🚀🔥🛸.txt");

Don't release Node.js while they fail.

What alternatives have you considered?

Something like this:
https://github.com/oven-sh/bun/blob/3221bfeeb7036e76872c1a602aa5cd7c83ed3b4a/test/js/node/fs/fs.test.ts#L681-L701

@AlttiRi AlttiRi added the feature request Issues that request new features to be added to Node.js. label Feb 17, 2024
@mertcanaltin
Copy link
Member

I think a file name with emojis will not be healthy in development processes

@AlttiRi
Copy link
Author

AlttiRi commented Feb 17, 2024

Node.js is only for the "development process" now?

What is the "development process"? And why while this "development process" I can't access the files with the legal filenames?

Node.js must not be used for desktop (Electron, NW and console) applications?


There is a file system bug on Windows.
Fix it and add tests to prevent it from regression in future.
If there were tests, then the breaking regression bug #48673 would not have appeared, which has not been fixed in LTS yet.

@anonrig
Copy link
Member

anonrig commented Feb 18, 2024

Any PRs are welcome

@mertcanaltin
Copy link
Member

Node.js is only for the "development process" now?

What is the "development process"? And why while this "development process" I can't access the files with the legal filenames?

Node.js must not be used for desktop (Electron, NW and console) applications?

There is a file system bug on Windows.
Fix it and add tests to prevent it from regression in future.
If there were tests, then the breaking regression bug #48673 would not have appeared, which has not been fixed in LTS yet.

I understand better now, sorry, I thought it was just a name feature with emojis.

@mertcanaltin
Copy link
Member

I'll be opening a pr trying to solve this

@mertcanaltin
Copy link
Member

I created a test for it and everything looks good
If there is something missing or something you would like to see improved, please do not hesitate

@AlttiRi
Copy link
Author

AlttiRi commented Feb 18, 2024

Yeah, the test fails on 20.11.1 LTS and passes on 21.6.2 Current as expected.


UPD.

node-v20.11.1-x64.msi:
20.11.1 LTS

node-v21.6.2-x64.msi:
21.6.2 Current

@mertcanaltin
Copy link
Member

I ran my test in the version you said and there was no problem. @AlttiRi

v20.11.1
➜  node git:(dev-51789) node test/common/file-ops-with-surrogate-pairs-on-windows.js
▶ File operations with filenames containing surrogate pairs on Windows
  ✔ should write, read, and delete a file with surrogate pairs in the filename (19.227042ms)
▶ File operations with filenames containing surrogate pairs on Windows (20.333584ms)

ℹ tests 1
ℹ suites 1
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 25.379917
➜  node git:(dev-51789) ```

@anonrig
Copy link
Member

anonrig commented Feb 19, 2024

cc @nodejs/platform-windows

@anonrig anonrig added the windows Issues and PRs related to the Windows platform. label Feb 19, 2024
marco-ippolito pushed a commit that referenced this issue Feb 29, 2024
PR-URL: #51800
Fixes: #51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito pushed a commit that referenced this issue Feb 29, 2024
PR-URL: #51800
Fixes: #51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this issue Mar 1, 2024
PR-URL: nodejs#51800
Fixes: nodejs#51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit that referenced this issue Mar 1, 2024
PR-URL: #51800
Fixes: #51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this issue Mar 1, 2024
PR-URL: nodejs#51800
Fixes: nodejs#51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this issue Mar 4, 2024
PR-URL: nodejs#51800
Fixes: nodejs#51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this issue Mar 5, 2024
PR-URL: nodejs#51800
Fixes: nodejs#51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@AlttiRi
Copy link
Author

AlttiRi commented Mar 11, 2024

Was this test backported to 20.x?
Was the bug fixed on 20.x?

@richardlau
Copy link
Member

Was this test backported to 20.x? Was the bug fixed on 20.x?

#51976

richardlau pushed a commit that referenced this issue Mar 15, 2024
PR-URL: #51800
Backport-PR-URL: #51976
Fixes: #51789
Refs: #48673
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
PR-URL: nodejs#51800
Fixes: nodejs#51789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants