Skip to content
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

test: fix flakiness of stringbytes-external #6039

Closed
wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Apr 4, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

test

Description of change

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: #5945
R=@bnoordhuis, @Trott
CI: https://ci.nodejs.org/job/node-test-pull-request/2146/
CI: https://ci.nodejs.org/job/node-test-pull-request/2147/
CI: https://ci.nodejs.org/job/node-test-pull-request/2148/

@jasnell jasnell added test Issues and PRs related to the tests. lib / src Issues and PRs related to general changes in the lib or src directory. lts-watch-v4.x labels Apr 4, 2016
@ofrobots ofrobots force-pushed the stringbytes-external branch 2 times, most recently from 9f7fe64 to 7ae1e9c Compare April 4, 2016 16:25
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
@Trott
Copy link
Member

Trott commented Apr 4, 2016

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

@ofrobots
Copy link
Contributor Author

ofrobots commented Apr 4, 2016

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.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

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

@gibfahn
Copy link
Member

gibfahn commented Apr 5, 2016

@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.

@evanlucas
Copy link
Contributor

LGTM

@jbergstroem
Copy link
Member

Would perhaps the cctest suite be a better fit?

@ofrobots
Copy link
Contributor Author

ofrobots commented Apr 5, 2016

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.

@Trott
Copy link
Member

Trott commented Apr 5, 2016

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
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
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>
@ofrobots
Copy link
Contributor Author

ofrobots commented Apr 5, 2016

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

@thefourtheye
Copy link
Contributor

Closing, as the changes have landed already.

@ofrobots
Copy link
Contributor Author

ofrobots commented Apr 6, 2016

Follow-on: #6073.

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>
@MylesBorins
Copy link
Contributor

@ofrobots this is not merging cleanly onto v5.x, would you be able to backport?

@MylesBorins
Copy link
Contributor

since this is not landing on v5.x I'm marking as dont-land-on-v5.x

We will still want this backported to v4.x-staging though

ofrobots added a commit to ofrobots/node that referenced this pull request May 12, 2016
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
Ref: nodejs#6039
Ref: nodejs#6073
MylesBorins pushed a commit that referenced this pull request May 13, 2016
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: #5945
Ref: #6039
Ref: #6073

PR-URL: #6705
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
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: #5945
Ref: #6039
Ref: #6073

PR-URL: #6705
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants