Create vee-eight-5.0 branch (take 2) by ofrobots · Pull Request #5945 · 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

Conversation15 Commits2 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 }})

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]?

Affected core subsystem(s)

deps, test

Description of change

Continuation (takeover) of #5592 as @targos isn't available in the next couple of weeks.

Updated to the latest 5.0-lkgr.

R=@bnoordhuis, @nodejs/v8
CI: https://ci.nodejs.org/job/node-test-pull-request/2087/

LGTM. No real suggestions for debugging the tests, I'm afraid. Perhaps check whether --gc_interval=1 makes a difference?

cc @nodejs/node-chakracore

I investigated the stringbytes test on the arm machines. It seems that they have indeed become flaky on the pi2 arm machines. With V8 5.0, the memory usage has changed subtly enough that common.enoughTestMem occasionally claims that we have enough memory to test, but an actual malloc done by ExternalStringTwoByte::NewFromCopy occasionally fails to allocate memory. Only the pi2 arm machines seem to be at this memory 'sweet spot' (or is it 'sour spot'?).

/cc @Trott because you have looked at flakiness of these tests in the past. One idea may be to adjust the threshold we use in common.enoughTestMem.

One idea may be to adjust the threshold we use in common.enoughTestMem.

That's probably the way to go unless/until someone has a better idea.

I have an idea to fix the flakiness of these tests, but I will have to move them to test/addons. Are test/addons ever run as part of the CI?

@ofrobots addons tests are run in CI on all platforms.

/cc @nodejs/build in case I'm misinformed.

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

Apr 4, 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

Thanks. Here's a PR to fix the flaky tests: #6039. I will rebase once that lands.

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

It seems there is a wayward flaky stringbytes-external test-cases was lurking in the parallel directory instead of sequential directory that #6039 missed. 😭

V8 5.0 introduced a small modification for the unexpected end of input error.

PR-URL: nodejs#5945 Reviewed-By: bnoordhuis - Ben Noordhuis info@bnoordhuis.nl Reviewed-By: indutny - Fedor Indutny fedor.indutny@gmail.com

@ofrobots ... as we discussed yesterday at the vm summit, will you be opening a PR to get v5 merged into master?

@nodejs/ctc ... @ofrobots and I spoke a bit yesterday about the possibility of getting v8 beta updates landed into master more regularly before they go stable so long as CI is green. Specifically, I'd like to get the v5 beta into the v6 release candidate that I'll be putting together on Monday.. so it would be fantastic if we can get v5 into master even tho it hasn't gone completely stable yet.

PR for getting V8 5.0 into master: #6111.

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

Apr 14, 2016

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

Apr 14, 2016