-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix multithreading to work in Deno and Bun #25947
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -398,6 +398,15 @@ def require_node(self): | |
| self.require_engine(nodejs) | ||
| return nodejs | ||
|
|
||
| def get_node_test_version(self, nodejs): | ||
| override = os.environ.get('OVERRIDE_NODE_JS_TEST_VERSION') | ||
| if override: | ||
| override = override.removeprefix('v') | ||
| override = override.split('-')[0].split('.') | ||
| override = tuple(int(v) for v in override) | ||
| return override | ||
| return shared.get_node_version(nodejs) | ||
|
Comment on lines
+401
to
+408
|
||
|
|
||
| def node_is_canary(self, nodejs): | ||
| return nodejs and nodejs[0] and ('canary' in nodejs[0] or 'nightly' in nodejs[0]) | ||
|
|
||
|
|
@@ -440,7 +449,7 @@ def try_require_node_version(self, major, minor = 0, revision = 0): | |
| nodejs = self.get_nodejs() | ||
| if not nodejs: | ||
| self.skipTest('Test requires nodejs to run') | ||
| version = shared.get_node_version(nodejs) | ||
| version = self.get_node_test_version(nodejs) | ||
| if version < (major, minor, revision): | ||
| return False | ||
|
|
||
|
|
@@ -625,7 +634,7 @@ def setUp(self): | |
|
|
||
| nodejs = self.get_nodejs() | ||
| if nodejs: | ||
| node_version = shared.get_node_version(nodejs) | ||
| node_version = self.get_node_test_version(nodejs) | ||
| if node_version < (13, 0, 0): | ||
| self.node_args.append('--unhandled-rejections=strict') | ||
| elif node_version < (15, 0, 0): | ||
|
|
||
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.
Can you remind my why this change is necessary? I can see that it might be needed, but it also seem unfortunate that it would be.
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.
The pthread tests look at JS_ENGINES and require that Node is used and that it's at least a specific version.
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.
Ah I guess I was hoping your OVERRIDE_NODE_JS_TEST_VERSION thing would by pass that check.
This change LGTM then, I can take a look at cleaning this stuff up as a followup.