test: do not spawn rmdir in test-statwatcher

[joaocgreis]

Refs: #21425

When running async-hooks/test-statwatcher locally in a loop, the first invocation always succeeds and all others fail. I see the same thing happening in recent stress tests (https://ci.nodejs.org/view/All/job/node-stress-single-test/2230/nodes=win2016-vs2017/consoleFull).

This is related to the presence of the tmpdir. Deleting the directory manually makes the next test run pass. This change prevents tmpdir.refresh() from spawning rmdir, and makes the test pass.

I don't know if this will fix #21425. Doesn't seem related to the older failures listed there, but might be related to the failures that are happening so often now. At least I'm hopping this will help debugging locally and produce more relevant stress test runs.

I'm not familiar with async hooks, I don't know if spawning interferes by design or is an indication of a real bug. Hence, I don't know if we should land this or keep investigating. @addaleax (by looking at file history) if you have an opinion about this it would be welcome, thanks!

Stress test: https://ci.nodejs.org/view/All/job/node-stress-single-test/2231/nodes=win2016-vs2017/consoleFull

cc @nodejs/testing

Checklist

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

[nodejs-github-bot]

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23963/ ✔️

[Trott]

Looks like it resolves the worst of the flakiness for test-statwatcher. There's still at least one other cause lurking in there somewhere, but one step at a time. 👍

[Trott]

Given the vastly improved CI results, I'd like to fast-track this. If you're a Collaborator, please 👍 here to approve fast-tracking.

[addaleax]

I don’t think I have much to add besides what @Trott said. :)

But as a side note, looking at 59f666c, it seems like ${pathname} inside the execSync call should be put into quotes or otherwise escaped? But I’m not on a Windows machine rn, so I can’t verify that that’s actually an issue. (I also don’t want to make a lot of fuzz about it for $reasons.)

[Trott]

Landed in 82fe33f

[joaocgreis]

Thanks for the reviews and @Trott for landing!

it seems like ${pathname} inside the execSync call should be put into quotes or otherwise escaped?

Indeed it should, Node can even do that automatically. But last time I checked Node couldn't be built on a path with spaces, so this is very low priority.

[Trott]

it seems like ${pathname} inside the execSync call should be put into quotes or otherwise escaped?

Indeed it should, Node can even do that automatically. But last time I checked Node couldn't be built on a path with spaces, so this is very low priority.

I don't have a Windows machine handy so I can't easily test, but if this looks right, I'll happily open the PR: Trott@2edb604

Or someone else can do it.

Or we can not bother for now for the reasons @joaocgreis cites.