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

failing test-trace-events-dynamic-enable #78

Closed
targos opened this issue Sep 5, 2018 · 9 comments
Closed

failing test-trace-events-dynamic-enable #78

targos opened this issue Sep 5, 2018 · 9 comments

Comments

@targos
Copy link
Member

targos commented Sep 5, 2018

This test was added in nodejs/node#22114

That PR backported https://chromium-review.googlesource.com/1161518 from V8.

The problem is that the CL was reverted in https://chromium-review.googlesource.com/1162122 and never relanded.

/cc @ofrobots

=== release test-trace-events-dynamic-enable ===
Path: parallel/test-trace-events-dynamic-enable
/home/mzasso/git/nodejs/canary/test/common/index.js:760
const crashOnUnhandledRejection = (err) => { throw err; };
                                             ^

AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:

0 !== 1

    at test (/home/mzasso/git/nodejs/canary/test/parallel/test-trace-events-dynamic-enable.js:54:10)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:270:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:745:3)
Command: out/Release/node /home/mzasso/git/nodejs/canary/test/parallel/test-trace-events-dynamic-enable.js
@ofrobots
Copy link

ofrobots commented Sep 6, 2018

I'll take a look. I am working on relanding the original change upstream. I had to fix some thread-safety issues (unrelated to the nodejs/node#22114, but exposed by it) that caused the revert. The upstream fixes that have landed: v8/v8@602aeb4, and v8/v8@f964846. I am still working on the reland: https://chromium-review.googlesource.com/c/v8/v8/+/1188529

@targos
Copy link
Member Author

targos commented Sep 6, 2018

Thanks for the update!

@targos
Copy link
Member Author

targos commented Sep 14, 2018

Any progress? I see there was some trouble with the reland CL

@ofrobots
Copy link

@targos I'm looking at this now.

@ofrobots
Copy link

The ASAN issues in V8 CQ were legitimate, and should be fixed by this fix: https://chromium-review.googlesource.com/c/v8/v8/+/1228419. Here are the steps that need to happen:

  1. Land the above mentioned fix.
  2. Rebase https://chromium-review.googlesource.com/c/v8/v8/+/1188529 on top of the fix. It should land cleanly this time.
  3. Pick up all these patches here.

Until then, we can either revert the change that introduced the test in Node, or disable it, like you have already. I'll let you decide on the approach.

targos added a commit to refack/node that referenced this issue Sep 18, 2018
@ofrobots
Copy link

Both changes have landed upstream.

@targos
Copy link
Member Author

targos commented Sep 19, 2018

Nice! Can we try to have the changes backported upstream?

@targos
Copy link
Member Author

targos commented Sep 19, 2018

LMK if I should open an issue for the merge request

@targos
Copy link
Member Author

targos commented Sep 19, 2018

Merge request opened: https://bugs.chromium.org/p/v8/issues/detail?id=8199

@targos targos closed this as completed Sep 20, 2018
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

No branches or pull requests

2 participants