Standardize CLI tests with bin tester package

[scalvert]

This PR is BIG, but contains no functional changes to the CLI itself. The changes are isolated to the test infra, and achieves a number of things:

1. Standardizes how we invoke tests for the bin, removing all other variants of bin test helpers
2. Converts all fixturify related APIs to async, ensuring we're correctly utilizing the new version of fixturify-project

[NullVoxPopuli]

Watching this, as this is a blocker for monorepo support, and it appears the entire v4 version of template-lint won't be compatible with monorepos (because my pr needs tests, and the tests need this pr)

Thanks for updating all the things! 🥳

[bmish]

If a version of @scalvert/bin-tester could be released with Node 12 support (which is not end-of-life for a few weeks still), then we could get this testing change in without needing a breaking change, and unblock @NullVoxPopuli critical monorepo plugin fix in ember-template-lint v4. I think it's important to get this issue fixed in ember-template-lint v4 since many users will likely be on that major version for a while.

[scalvert]

I'm not 100% sure I can, since bin-tester depends on some packages that are node 14 up, specifically vitest (<- you know what I'm talking about @NullVoxPopuli!).

Let me see if I can figure things out and will update this accordingly.

[rwjblue]

It's probably find to just wait for Node 12 EOL in a couple weeks.

unblock @NullVoxPopuli critical monorepo plugin fix in ember-template-lint v4

How is this PR (which is just an internal testing refactor) blocking @NullVoxPopuli's monorepo fixes? They seem unrelated, if that PR is based on this one (for it's ease of testing) it can be updated to do what we do in the tests before this PR. There is no need for them to be coupled as far as I can tell.

[rwjblue]

Watching this, as this is a blocker for monorepo support

@NullVoxPopuli - This statement makes no sense to me, can you explain it?

[NullVoxPopuli]

@NullVoxPopuli - This statement makes no sense to me, can you explain it?

I was told that my PR needs to use fixturify-project to create a monorepo in the tests, which needed APIs in fixturify-project@v4+, which was only partially migrated to, iirc -- and what this PR completes.

(there are existing tests that I can migrate once the project is ready for that)

however

@bmish declared that

Tests for this should hopefully be unblocked now that this has been merged:

however however, in discord,

me:

I need to rebase that PR, because I think someone got fixturify project upgraded? (so I can add more reasonable tests now?)

@scalvert

well fixturify-project was only partially updated
that's partly what my PR above fixes
but also, i just released a new fixturify-project version due to a bug in 4.1.1
and have subsequently upgraded it in my bin tester PR
so i think we should try to get that merged soon

---

So I interpreted that as this PR, #2437, being a blocker to mine -- if that's not the case, I'm happy to update things in my PR (#2359)

[bmish]

By unblocking these concerning issues (which I'd like to get fixed during ember-template-lint v4):

1. Windows support: Convert path to URL before import of config/plugin for Windows support #2339
2. Monorepo support with plugins: Fix monorepo support by allowing import to do what it's good at (and … #2359

I was just referring to ensuring we can properly test them, and it seemed that the ability to conveniently write tests for them was blocked by some stalled test dependency upgrades.

Since these issues can require complex reproductions, I just want to ensure our tests actually cover these scenarios, so we can be confident that we fully fix them and that we don't end up breaking them again.

[rwjblue]

I was just referring to ensuring we can properly test them, and it seemed that the ability to conveniently write tests for them was blocked by some stalled test dependency upgrades.

Gotcha. See #2471 and #2473.

[scalvert]

@bmish @rwjblue I added backwards compat support for Node 12 to @scalvert/bin-tester. I've also rebased this branch on top of master, so it includes @rwjblue's new monorepo tests using bin-tester too (I had to add support for dynamic binPath options passed to bin-tester).

I think once CI passes we should get this merged.

[scalvert]

I'll revert the package.json change to engines that removes Node 12 support.

[bmish]

Code changes look good. Thanks for removing the breaking change.

[scalvert]

Once I get CI back up and running I'll merge. Thanks for reviewing!

[bmish]

Why are we adding this exception for to ignore the engine check for Node 12? That defeats the check to ensure we remain compatible with Node 12.

It looks like @scalvert/bin-tester has a dependency that is not compatible with Node 12. That is where I think we should fix the issue.

[scalvert]

Because it's a devDependency, not a dependency, so it's not material whether Node 12 is supported in that devDependency or not. The incompatibility is in fixturify-project, and that's not going to be downgraded since it was explicitly updated to make new APIs available to bin-tester, which in turn are required by ember-template-lint.

Adding this escape hatch to CI simply avoids this issue for our tests, but doesn't negatively impact any code destined for the published package. This coupled with the short duration that we have to support Node 12 felt like an acceptable tradeoff.

[bmish]

I agree of course that dev dependencies like this won't break our Node 12 support. I wish there was a way to distinguish dev vs. non-dev dependencies in the Node engine check during CI, but I haven't seen one, so it's normal for projects to end up prevented from upgrading dev dependencies until they drop old Node version support.

If you really want to get this in now, I'm fine with you to proceed with modifying this check to be more lenient. This PR does look like a big improvement. I just want to call out the risks.

In particular, it could be several months before we get the next major release (#2319) out as we finalize and implement its changes and depending on maintainer bandwidth. The last major release (which was admittedly huge) took a few months to complete. In the meantime, we might accidentally merge Dependabot PRs of dependencies that drop Node 12 support, since the CI check won't catch them anymore. So we would have to be especially mindful (added burden for us) in the coming months to avoid breaking Node 12 support.

[scalvert]

Ya I appreciate what you're saying, @bmish. I also appreciate all the work you've done on the repo in the last while!!

I do agree there's some potential risk associated with this. I feel like it's inline with the risk associated with skipping those two test files before Node 16 - it's not ideal, but intended to be short-lived.

I'll see if I can get CI back up and running, and for now I've separated Node 12 into its own job so that it can be easily removed once we remove it.

[bmish]

Sounds good, thanks!