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

test: run node_unit_tests with DENO_FUTURE=1 #25285

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Aug 28, 2024

This is blocking #25213.

Turns out a bunch of FS APIs are completely broken because they
use RIDs (resource IDs) instead of FDs (file descriptors).

@@ -64,6 +64,9 @@ Deno.test({

Deno.test({
name: "Async: Data is written to passed in rid",
// TODO(bartlomieju): this test is broken in Deno 2, because `file.rid` is undefined.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about these TODOs being lost and not taken care of before the release. Maybe we should label them like TODO(2.0)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsherret this is not really a Deno 2 issue. These APIs are just outright broken and have always been. They're covered in #23012.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll link back that PR in the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the test was previously passing and now it's not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't thinking. This seems find to me.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I should be able to help clean this up once we're ready.

@iuioiua iuioiua changed the title test: run node_unit_tests with DENO_FUTURE=1 test: run node_unit_tests with DENO_FUTURE=1 Aug 28, 2024
@bartlomieju bartlomieju enabled auto-merge (squash) August 28, 2024 23:58
@bartlomieju bartlomieju merged commit 52fb658 into denoland:main Aug 29, 2024
17 checks passed
@bartlomieju bartlomieju deleted the test_node_uniot branch August 29, 2024 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants