Revert "test: improve async-hooks/test-callback-error"

[Trott]

This reverts commit 32c7f11.

That change seems to cause make test (and make test-async-hooks) to fail 100% of the time on macOS.

Possible reimplementation would be to move the new test cases to it's own file and skip macOS on it, while also having it in known_issues where only macOS runs it. And in a separate commit, mark the previous incarnation of the test flaky on OS X. But first thing first--let's get the build working again. (Although the commit being reverted here turns CI perma-yellow rather than perma-red, it breaks workflow locally.)

@nodejs/platform-macos

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

test async_hooks

[Trott]

/cc @refack @AndreasMadsen

[Trott]

@nodejs/testing

[Trott]

I'm inclined to land this one very quickly and not wait the 48 hours. Thoughts?

[Trott]

If any @nodejs/collaborators who use macOS can confirm that make test or make test-async-hooks is broken for them on current master but works with 32c7f11 reverted, that would be helpful to know. I mean, I suppose there's a chance this is all Just Me. I don't think so, though....

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/8776/

[aqrln]

@Trott both make test and make test-async-hooks work for me on master.

[Trott]

@aqrln Ah! I realize now this comes down to whether or not your macOS will delay exiting with --abort-on-uncaught-exception or not. I suspect (but don't know) that it tends to affect older macOS more than recent macOS. I'm running 10.11. I think CI runs 10.10. Are you running 10.12 or 10.13 by any chance?

[aqrln]

@Trott yes, I'm running 10.12.5.

[Trott]

If what's going on in #13527 (comment) can be figured out, it's possible that it may reveal a better path forward than a revert...

[AndreasMadsen]

We have tested this very extensively, the new test doesn't change anything, it is just a better written test. I very much doubt the CI is failing, since we explicitly set it to PASS, FLAKY on both MacOS and Linux.

If you are experiencing issues then please help us debug it. Neither @refack or I have the tools to find the root cause of this issue.

[refack]

We have tested this very extensively, the new test doesn't change anything

There is 1 change, timeout was reduced from the default 60sec to 15sec.

[AndreasMadsen]

There is 1 change, timeout was reduced from the default 60sec to 15sec.

True, we can set to back to 60sec. Although, on my machine the test consistently uses between 0.2s and 0.4s (10000 runs). So I would argue that if 60sec passes and 15sec don't, that is just a false positive.

[refack]

There is 1 change, timeout was reduced from the default 60sec to 15sec.

True, we can set to back to 60sec. Although, on my machine the test consistently uses between 0.2s and 0.4s (10000 runs). So I would argue that if 60sec passes and 15sec don't, that is just a false positive.

I'm just being pedantic.
BTW: @Trott AFAIK is hard at work figuring this out, since outside of test-requireio-osx1010-x64-1 he has the only reproductiong machine

[benjamingr]

@Trott both make test and make test-async-hooks work for me on master.

Hi, got a mac, can confirm the same

[Trott]

@benjamingr What version of OS X/macOS are you running?

[Trott]

I very much doubt the CI is failing, since we explicitly set it to PASS, FLAKY on both MacOS and Linux.

FWIW, I did not say CI is failing. In fact, I said CI is yellow. "Although the commit being reverted here turns CI perma-yellow rather than perma-red, it breaks workflow locally."

I used the word "fail" only in regard to make test and make test-async-hooks. The .status files are not honored by make test. CI runs make test-ci rather than make test.

If you are experiencing issues then please help us debug it.

I've been debugging it on-and-off all day, although most of the significant work was done by @refack and @addaleax instructing me to do things over IRC. (See #node-dev if you want Too Much Detail. For more signal/less noise than IRC, I've been posting the interesting things in the issue for this test: #13527)

[trevnorris]

@Trott Any details on whether the change still needs to be reverted?

[Trott]

@Trott Any details on whether the change still needs to be reverted?

I don't think we need to revert, but I was going to leave this open until #13865. Not sure this revert would address that issue, though, so maybe closing this makes sense. Either way, really.