use process.execSync in favor of execa in tests

[bcoe]

process.execSync is great for writing unit tests, it's what we use in Node.js itself.

I think that execa, cross-spawn, etc., address some historic inconsistencies in the Node.js API that we shouldn't be bumping into in simple assertions (and I'm not sure what, if any of the issues, continue to exist on Node.js > 6).

[JustinBeckwith]

👋 so this is probably mostly my fault. I got into a "everything should be async all the time" phase, and probably went a little overboard in extending that to tests as well. I'm totally supportive of making this change, as long as we have a plan.

We have samples across 62 repos.... what's a good way to look at executing on updating them? Can we use repo for this I wonder?

[jkwlui]

Also a lot of function will now be synchronous, so not a easy mass-change if we just swap out the call.

[JustinBeckwith]

I dunno...

Replace:

 = await exec(

with

 = execSync(

that, and deleting references to execa. Is there more to this?

[bcoe]

@JustinBeckwith @jkwlui in the Node.js tooling WG, we can't honestly figure out amongst a bunch of folks writing tooling what's actually broken in child_process anymore, because lots of folks in the community moved to using execa, cross-spawn, etc., years ago.

Mainly it would add less of an abstraction between our libraries and the Node.js platform, and in turn benefit the platform -- if we do find a bug with spawn or spawnSync, I'll happily patch it in Node.

[JustinBeckwith]

I'm 100% supportive. Just trying to figure out how to do it in a fell swoop :)

[bcoe]

https://github.com/bcoe/awesome-cross-platform-nodejs#known-issues

☝️ document I wrote years ago that outlines some of the edge-cases in spawn, it would be good to make sure it's up to date, i.e., I notice one of the referenced issues is closed.

[bcoe]

there's a conversation happening here about the remaining deltas between execa and spawn:

bcoe/awesome-cross-platform-nodejs#26

I would love to address these in the Node.js tooling initiative: https://github.com/nodejs/tooling

[JustinBeckwith]

So rolling list of feedback from me so far:

• Needing to be explicit about utf8 encoding so I don't get a Buffer was mildly irritating. I can't figure out when it decided to coerce into a string and when I got a Buffer 🤷‍♂️
• @jkwlui and I spent a comical amount of time figuring out that execa was stripping whitespace characters for us from console output, and execSync does not. That was a doozey!

[bcoe]

@JustinBeckwith the {"encoding": "utf8:"} is annoying, but at least consistent with the readFileSync 🤷‍♂️.

The additional \n is a pain!, I don't love the idea of writing code as:

execSync('echo foo', {encoding: 'utf8'}).trim()

☝️ that's starting to get a bit verbose right? I don't love that the test assertions are so strict on exact matches any ways, I'd love to convert a couple tests to using snap-shots for CLI output, just to show what this methodology looks like -- it makes this type of assertions less fragile.

[JustinBeckwith]

Callin this one a win 🙃