-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
test/parallel/test-tick-processor-arguments
fails on SmartOS with V8 >=11.8
#50050
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
Comments
targos
added a commit
to targos/node
that referenced
this issue
Oct 5, 2023
targos
added a commit
to targos/node
that referenced
this issue
Oct 7, 2023
SGTM |
targos
added a commit
to targos/node
that referenced
this issue
Oct 10, 2023
targos
added a commit
that referenced
this issue
Oct 10, 2023
Refs: #50050 PR-URL: #49639 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
alexfernandez
pushed a commit
to alexfernandez/node
that referenced
this issue
Nov 1, 2023
Refs: nodejs#50050 PR-URL: nodejs#49639 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
debadree25
pushed a commit
to debadree25/node
that referenced
this issue
Apr 15, 2024
Refs: nodejs#50050 PR-URL: nodejs#49639 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
I think disabling that test on SmartOS is an expedient solution for now. I took a look at fixing the test on SmartOS, but patching the V8 tooling to support BigInt for that test case just broke the many other parts of V8 that currently rely on parseInt behaviour.
I've raised https://bugs.chromium.org/p/chromium/issues/detail?id=1486517 with the V8 team to see if they have any useful input, but the proper fix for this looks like a reasonable amount of work that doesn't feel like it should be our responsibility to fix, and given the amount of parseInt hardcoding currently in the V8 tools and the fact they deliberately only have partial BigInt support I don't know that our work on a fix would even be accepted upstream.
As I mentioned in the other bug, node itself appears to work absolutely fine with the V8 merges, it's just this single test case with a clear bug in the V8 tooling scripts, so while not ideal I don't think it should hold up this merge, especially as the number of SmartOS users who use V8 log parsers is probably at or close to zero ;-)
Originally posted by @jperkin in #49639 (comment)
The text was updated successfully, but these errors were encountered: