Skip to content

Conversation

@dfavretto
Copy link
Contributor

Proposed Changes

Separate tests execution for each module to prevent Allocation Fail Error.

Using Node JS versions higher than 10.8.0 throws an Allocation Fail Error when running the tests due to a bump in the V8 version from 6.7 to 6.8 (introducing a change in heap memory limit).
Because of this, we separated the execution of the tests for each module in different scopes; and doing so, with the command npm run test, AVA executes the tests from different index files.

image

Testing

  1. IMPORTANT: Use a Node JS version greater than 10.8.0.
  2. Go to JavaScript directory.
  3. Execute lerna clean --yes
  4. Execute npm install.
  5. Execute npm run build.
  6. Execute npm run test.
  7. Check that all test passed.

Evidence

A comparation was made before and after the modifications for NodeJS version 10.9.0 and the last stable version at this moment 11.5.0.

v10.9.0

Before
image

After
image

v11.5.0

Before
image

After
image

@tellarin
Copy link
Collaborator

tellarin commented Jan 4, 2019

@dfavretto @GasparAcevedoZainSouthworks This change is good, but is there a way to set a higher heap limit in V8? I wonder if package usage outside of tests would face this issue.

@gasper-az
Copy link
Contributor

gasper-az commented Jan 4, 2019

@dfavretto @GasparAcevedoZainSouthworks This change is good, but is there a way to set a higher heap limit in V8? I wonder if package usage outside of tests would face this issue.

@tellarin, we have researched on how to increase the V8 heap limit and we found that the command --max-old-space-size sets the heap size into a specified amount of memory (in MB).
The basic syntax to use this command is node --max-old-space=size={size} {file}, where {size} is the heap memory limit in MB.
But, as in the Recognizers-text/JavaScript the tests are executed through AVA, we changed it a little and executed this command with the profile.js file (the file AVA runs):
node .\node_modules\ava\profile.js --max-old-space-size=8192 .\test\index.js.
We executed this 10 times, but only one of those executions concluded successfully, which means that using this method in the project is not feasible:

image

The others test throw the same heap out of memory error:

image

So far, this is the only way to increase the heap memory size we have found.

@tellarin
Copy link
Collaborator

tellarin commented Jan 7, 2019

@GasparAcevedoZainSouthworks, thanks for the reply. Can you also confirm that the SampleConsole works fine in multiple cultures after the bump?

@gasper-az
Copy link
Contributor

@tellarin, we have tested the simple-console sample with both NodeJS versions v10.3.0 and v10.15.0 using some of the examples provided by the sample and the results were the same for both tests executions. Here are some screenshots as evidence:

image

image

Is this useful for you or need more evidence to continue merging the PR?
Thanks!!!

@tellarin
Copy link
Collaborator

tellarin commented Jan 8, 2019

@GasparAcevedoZainSouthworks I've pulled the branch and run similar tests in French and all seems good. I'll merger this now. Thanks!

@tellarin tellarin merged commit da9bc5c into microsoft:master Jan 8, 2019
@JuanAr JuanAr deleted the southworks/fix-allocation-failed branch January 9, 2019 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants