fix(node): Enable native node tests

[Soremwar]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[Soremwar]

This will implement https://github.com/Soremwar/deno_node_tests

This is gonna be a big refactor, many tests won't be enabled as of now due to possible incompatibilities between the node lib and /std/node but luckily I designed the test script to be very configurable allowing to us to choose what tests to bring in

[ry]

Sorry I'm a bit confused about the purpose of this. Can you explain more what you had in mind?

[Soremwar]

This will allow us to test std/node with the actual test suite the node folks use in their repo. This should help catch any implementation bugs or lack of features that the current library might have (I know for a fact this kind of inconsistencies are present in the Buffer implementation)

[Soremwar]

I'm done for the night, I need to actually find some tests we can enable to start using this but it should be good to go for review

[Soremwar]

BTW How should we handle the node license here? I can append the MIT license to the top of each file but IDK if that's enough

[ry]

Regarding license, you do not need to include the full text of the license at the top of each file, just a single line

// Copyright Joyent and Node contributors. All rights reserved. MIT license.

[Soremwar]

Test suite functional and configurable, well documented and with the proper copyright headers.

Big plus, I just set up the first test and catched the first implementation inconsistency (a static method inside EventEmitter)

This is ready for review

[caspervonb]

Would prefer if this was more like regular tests and didn't require special steps to test.
WASI does this too with upstream tests but re-uses Deno.test.

[ry]

Have you considered including Node as a git submodule? Is it too big?

[Soremwar]

A git submodule could suffice, but it makes it harder to patch some files that are needed for this to work, namely common/index.js.

That file is used in pretty much every single file, however it uses things like child_process that are not needed in most cases, so instead I just patch it and all the tests that are enabled continue to use it without any trouble

Not to mention this is more user friendly than submodules :)

[kt3k]

How about mocking common/index.js by injecting a fake module into require.cache, using a technique described here?

[Soremwar]

That might be more complicated than the current solution though @kt3k

[kt3k]

Tested locally. I was able to add test cases by adding items in config.json and running setup.ts 👍

setup.ts looks good to me. It requires the internet access, but that's only in the contributors' machine who want to change the node compatibility test. It doesn't affect CI and other contributors. setup.ts is also good in the sense it uses the fixed version of node.js and clear all the previously downloaded cases. So the result is deterministic and idempotent. I think there will be less (or no) confusions about updating these test cases.

BTW the directory name for these tools look a bit strange to me. I think we usually use testdata for just data for testing (like fixtures). Because setup.ts is the entrypoint for the developers, how about naming like /node/tools/setup.ts? @Soremwar

[ry]

I also tested it locally and it works. I'm in favor of this change. Nice work @Soremwar!

BTW the directory name for these tools look a bit strange to me. I think we usually use testdata for just data for testing (like fixtures). Because setup.ts is the entrypoint for the developers, how about naming like /node/tools/setup.ts?

This also stood out to me. Non-fixture stuff should not be in testdata.

[Soremwar]

node/tools sounds good to me, just updated the test folder

[lucacasonato]

Maybe node/_tools because it is internal?

[Soremwar]

_tools it is

[kt3k]

LGTM

[caspervonb]

LGTM 😁

[kt3k]

@Soremwar Thank you for your contribution! Merging.