-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Skip other.test_pthread_growth test on Windows on ARM #25628
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
Conversation
| if WINDOWS and platform.machine() == 'ARM64': | ||
| # https://github.com/emscripten-core/emscripten/issues/25627 | ||
| # TODO: Switch this to a "require Node.js 24" check | ||
| self.require_node_canary() |
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.
Should this go in the else block below because the if branch already requires node canary.
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.
Hmm how do you mean the if branch already requires node canary?
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.
I mean line 13000 below.
Since the GROWABLE_ARRAYBUFFERS already requires canary, and the comment here belngs instead in the else branch below.
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.
Right.. I think it probably reads cleaner to not intertwine this test skip/workaround with the growable arraybuffer logic part.
sbc100
left a comment
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.
lgtm % comment
|
Looks like has regressed. (ignored that to land) |
Already reverted. |
Skip other.test_pthread_growth test on Windows on ARM if Node.js is 22.16.0. See #25627
In the future, after #25533 lands, we can move towards a direction of auto-detecting and auto-detecting skipping per Node version.