update smoke-test-app to fix CI

[mansona]

I have also cherry picked the work from #896 because that was causing some issues with newer npm versions. I have no idea why this wasn't a problem before 🤷 🤪

Also I needed to update the smoke-test-app to get CI passing again

[codecov]

Codecov Report

Merging #893 (1d08ed0) into master (17592f8) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 1d08ed0 differs from pull request most recent head bc27a0d. Consider uploading reports for the commit bc27a0d to get more accurate results

@@           Coverage Diff           @@
##           master     #893   +/-   ##
=======================================
  Coverage   94.36%   94.36%           
=======================================
  Files          17       17           
  Lines         550      550           
=======================================
  Hits          519      519           
  Misses         31       31           

Impacted Files	Coverage Δ
smoke-test-app/config/ember-try.js	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kategengler]

0.0.1 never existed, so I wonder why it was there in the first place. Was it trying to test something non-existing? What version do the relevant tests / try configs use?

[mansona]

So we saw no evidence of a specific test trying to check for a missing version or anything like that 🤷 and it's just NPM that is failing on the standard tests.

I swapped the order of commits so you can see that the CI is failing without that cherry-pick fix https://github.com/ember-cli/ember-try/actions/runs/3436635236/jobs/5730381232#step:5:123

With that change all of the CI is now passing so personally I think it's good & necessary to include 👍

[kategengler]

I think we need a third version of ember-try-test-suite-helper. The configs in the tests use both 1.0.0 and 1.0.1

ember-try/test/tasks/try-each-test.js

Line 26 in 61fed6b

'ember-try-test-suite-helper': '1.0.0',

So that the scenarios aren't using the same version in the base package.json for the tests.

[mansona]

So I'm not entirely sure what you mean by this 🙈 I'm pretty sure we have deployed this to npm essentially just to test ember-try and there are only 2 versions deployed on npm https://www.npmjs.com/package/ember-try-test-suite-helper?activeTab=versions

Are you saying that we need to "deploy an new version to npm" so that we can verify that the configs are actually changing the version? Are you able to do this then since I don't have permission? I have access to the Ember CLI Team 1Password but it doesn't look like it has the ember-try user is in there. Could you add it or give the ember-cli npm user permission to deploy that package?

If you get a chance to do that I can actually deploy the 0.0.1 version and revert the change to this PR 👍

[kategengler]

Yeah, I think we should deploy a third version. I went to deploy one and noticed that there is ember-try-test-suite-helper-alternate that has a version of 0.0.1, so now I wonder if it were supposed to be that?

[mansona]

So I think it's going to be pretty hard to figure out why that line was there in the first place. We could try to divine it from evidence in the repo but there just isn't any 🤷 there is nothing testing that dependency before ember-try is run so it's not a before-and-after kinda situation, I just verified this by deleting the line in the fixture and running the tests locally and it works fine.

I personally see it this way: the value of getting CI green again is very high, and there seems to be no material difference with changing ember-try-test-suite-helper in the way that this PR does (originally cherry-picked from #896 ). If we're worried about confusion then we should probably delete the devDependency from the fixture and have it be an ember-try-only dependency that is being verified.

So that's what I think the choice should be:

a) merge this PR as is or
b) delete the dependency to avoid confusion

What are your thoughts?

[mansona]

I just removed the dependency and it works 🎉

[mansona]

Since #913 was merged already to drop EOL Node versions I removed that work from this PR and updated the title