Not specific to this test but this removes a key feature of deprecation testing. By having a CI job that simulates all deprecations as having past their 'until' we test that deprecations have been implemented in a way that works before and after the deprecation begins to throw:

https://github.com/ember-cli/ember-cli/blob/master/.github/workflows/ci.yml#L123-L133

This test was the way to guard to ensure OVERRIDE_DEPRECATION_VERSION was working

Any thoughts on how we can test OVERRIDE_DEPRECATION_VERSION? In the original we punted and put this test in so we'd at least see it and wonder.

And this one^

so I think I finally understand why I found it so hard to understand this comment. The test that you're commenting on really isn't testing what you are referring to, it's purely a mechanism test to make sure that the deprecation function throws correctly. I would argue that this is the upstream's job to test but it's fine to have a tests here to make sure we're happy that the new implementation does what is expected.

This looks like it's removing functionality, but it's not. We have the exact same functionality, it's just that we create a deprecate function in module scope now so we can't use an environment variable to live-update what semver-deprecate considers to be the current version. We still have the exact same test infrastructure before and after this change and we're still running the entire CI against a "super future version" of ember-cli because of this: https://github.com/ember-cli/ember-cli/pull/10610/changes#diff-c78d9b75e1a2b3b6515370b4643f590c305824fb4e59febbcdbee11ec5e208d1R6

it's just that we create a deprecate function in module scope now so we can't use an environment variable to live-update what semver-deprecate considers to be the current version.

That was not the part I objected to removing and while the original comment was covering the entire behavior (it predated the addition of that line in module scope), it was a test that ensured we don't accidentally remove the ability to override the version via an environment variable. We need some test to ensure we don't do that because it would be so easy to remove that line you linked and the CI job testing the future versions would continue to happily pass.

ah I get you. Since it's module scope the simplest thing I thought to do was to run a new node process as part of an integration test.

I'm pretty sure this is more coverage than the feature had before my changes 🤔 because the test that was removed actually wasn't really testing the code that ember-cli was using, just the underlying mechanism.

Let me know if that test matches what you were expecting 👍

Perfect

almost perfect 😂 the CI job that runs the whole suite with the override was (understandibly) messing with the test 🙈 I've fixed it now and CI should go green 👍

This function is still here but it was called in the code extracted. Is it used elsewhere?

I think this comment is still relevant

yes it's used in the lib/debug/deprecations.js file to create a convenience function to check if it was removed https://github.com/ember-cli/ember-cli/blob/master/lib/debug/deprecations.js#L9

I have updated semver-deprecate to expose this function so we can be sure that we are using the same implementation and we're just closing over the upstream now to make sure that we're using the correct version 👍