test: fix flakiness of stringbytes-external by ofrobots · Pull Request #6039 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation13 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

The tests used to rely on precise timing of when a JavaScript object would be garbage collected to ensure that there is enough memory available on the system. Switch the test to use a malloc/free pair instead.

Ref: nodejs#5945

Interesting. I believe all the other tests in addons are testing native addons. But in this case, you're building an addon so you can get some info to determine whether or not to skip a memory-intensive test that otherwise does not involve a native addon. If we want to go down this path, I wonder if we want to make that clear somehow. Not sure how, though. (And maybe it doesn't matter?) @nodejs/testing

Indeed, although my interpretation of addons test was 'anything that needs an addon to test'.

An uglier alternative would be to expose the EnsureAllocation function in node-core to be used by these types of tests only. I really would like to avoid adding this test-only useless APIs to core however.

This is certainly an interesting case. Overall it LGTM it's just definitely quite a bit different from our other tests.

@ofrobots @Trott

my interpretation of addons test was 'anything that needs an addon to test'.

If that's the consensus then presumably the tests README should be updated to clarify that. It currently says:

Tests for addon functionality.

Would perhaps the cctest suite be a better fit?

Adding a new suite for this is probably overkill. I don't think this pattern is going to be very common to require a suite.

I would like to land this PR today as it is blocking #5945 which needs to land very soon in order to get ready for the Node v6 RC schedule.

LGTM. After it lands, we can update the README in the test directory to call out this particular test.

ofrobots added a commit to ofrobots/node that referenced this pull request

Apr 5, 2016

Ref: nodejs#5945 PR-URL: nodejs#6039 Reviewed-By: jasnell - James M Snell jasnell@gmail.com Reviewed-By: evanlucas - Evan Lucas evanlucas@me.com Reviewed-By: Trott - Rich Trott rtrott@gmail.com

Thanks. Landed as f4ebd59. I will update the README in a follow-on.

Closing, as the changes have landed already.

ofrobots added a commit to ofrobots/node that referenced this pull request

Apr 6, 2016

Avoid depending on precise timing of when an object will be collected by GC. This test was missed by nodejs#6039 as it happened to be in a different directory than the rest.

Ref: nodejs#6039 PR-URL: nodejs#6073 Reviewed-By: Trott - Rich Trott rtrott@gmail.com