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

deno: reenable Deno CI tests #414

Merged
merged 2 commits into from
Mar 29, 2024
Merged

deno: reenable Deno CI tests #414

merged 2 commits into from
Mar 29, 2024

Conversation

guybedford
Copy link
Collaborator

Thanks to the work by @mash-graz the latest Deno release now supports far more of the test suite.

This reenables the Deno CI test suite and also enables more of the tests - we are now passing 64/106 tests in Deno.

@mash-graz
Copy link
Contributor

mash-graz commented Mar 29, 2024

I was just waiting for this new release before I send you some jco resp. preview2-shim patches to eliminate a few more compatibility issues,

Many of the reported errors are not very important in practice. Most of them are just reporting, that deno still reports port number 0 instead of the automatically chosen one and also for wildcard interface 0.0.0.0 a similar error will be displayed. But both cases are nearly negligible.

But other compatibility issues have to be fixed. For example deno doesn't support setting the TTL value for UDP packets. It will throw an NotImplemented Error, but jco doesn't catch this case, therefore every UDP command will fail...

Some more complex workarounds, which I had to figure out, are not necessary anymore, because the deno maintainers spend a lot of time improving the node:workers_threads implementation. Most of the grave defects of this node compatibility module are now solved in deno.

There is still one showstopper remaining: The file access in different threads resp. worker contexts doesn't work. I already reported it here (#400). We could work around this issue by utilizing the synckit worker indirection also for all file system related operations, but this would have negative consequences for the performance. @bartlomieju thinks, that this incompatibility can be also solved on denos side after another complex change. (see: denoland/deno#23012)

@guybedford
Copy link
Collaborator Author

Great to hear there may still be some low-hanging fruit. I've also disabled the Windows Deno tests for now as they had a few more errors as well.

It was fantastic to see the worker improvements - if anything this Jco compat does ensure Deno has very strong Node.js compat, so glad we can put pressure on that through these tests.

I'm open to your suggestions for any workarounds needed in Jco. Moving all filesystem descriptors to the worker could well be an option - alternatively we could even make that an implementation detail that is Deno-specific if it helps and this can't be fixed upstream.

@guybedford guybedford merged commit 7b41c35 into main Mar 29, 2024
8 checks passed
@guybedford guybedford deleted the deno branch March 29, 2024 01:20
@bartlomieju
Copy link

Hey @guybedford @mash-graz.

Thanks for the ping and thanks for surfacing this project to us. It's great seeing that we have 60% compatibility, but we'd like to get this number closer to 90% and 100% preferably. I'll schedule some time to figure out which tests are failing and we will keep our eye on denoland/deno#23012 as it seems like a must to get to that number.

@guybedford
Copy link
Collaborator Author

@bartlomieju it was really great to see this progress! So glad you are interested in seeing these worked through. I can also aim to do some more issue isolation in due course as these cases progress.

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.

None yet

3 participants