ChakraCore
ChakraCore copied to clipboard
chakra-core
Fix assertion failure in `JavascriptArray.cpp`
Sure, but it seems like I’ve created a new assertion failure; I’ll have another look at this…
Problem is, that the assertion error only occurred with extremely large arrays. Using the poc of the mentioned issue causes the test to timeout.
The timeouts are on a per JS file basis - does it still timeout if this test is done in a separate file with nothing else?
Could the test complete in 3 minutes? Normally each test file gets a 1 min timeout but tests run with the Slow tag get 3 minutes instead (and are limited to only a couple of the test builds though that's not the point here).
If even 3 minutes is too short I don't know what to do - an Assert we can't test seems pointless. @ppenzin thoughts?
I agree, we should try to run it as a separate test and mark as slow (given it does finish in 3 minutes).
I took a peek at the test - I think the key is separating this test from the reset of ES6 Array API tests by putting it into a separate JS file that would be marked as Slow. I've done that and the testing succeeded, though do we actually run these slow files? Question to @rhuanjl, I think. Both of my runs (debug and release with debug info) produced "exclude: slow" in the output.
I took a peek at the test - I think the key is separating this test from the reset of ES6 Array API tests by putting it into a separate JS file that would be marked as Slow. I've done that and the testing succeeded, though do we actually run these slow files? Question to @rhuanjl, I think. Both of my runs (debug and release with debug info) produced "exclude: slow" in the output.
You have to use a flag to run slow tests. Currently we do this on two of the windows CI builds x86 test and x64 test.
I should note I never actually checked whether the assertion was just a left-over from a refactoring or a valid check for the following code.
Edit: The fast path uses uint32 as indices, so the asserts have to ensure no overflow occurs.
@ppenzin It seems like the macro change hasn't been merged yet (See #6970). I gues their checks were failing because of a wrong copyright notice...
@ShortDevelopment did you catch if these were the new tests failing? Azure link sometimes opens and sometimes doesn't for me
@ppenzin Yes. Sadly the tests still timeout. I'm not sure whether I did apply the slow flag correctly...
I've tried that on Windows sometime ago, but can't remember how I did it, need to revisit.