search

ember-cli / ember-try

An ember-cli addon to test against multiple npm dependencies, such as ember and ember-data.
MIT License
179 stars 58 forks source link

ember try:ember seems to be doing a bit too much #851

Open mansona opened 2 years ago

mansona commented 2 years ago

I opened a PR a little while ago and I noticed that some of the tests were failing. Digging into it a little bit, it seems like the failure is to do with the ember try:ember command.

Looking at the results printout for the scenario ./node_modules/.bin/ember try:ember '3.2.0' --skip-cleanup=true it looks like it's running way more tests than just ember-source@3.2.0 🤔

Looking at the output it is running tests for

The first surprising thing to me is that it runs anything other than 3.2.0 🤔 I guess I could come to peace with the fact that it ran 3.2.0 as well as the scenarios that were defined in the ember-try config. But I can't see why this command would result in the beta, alpha, and release versions getting run 🤔

I suspect this is just a bug in the implementation. If you can confirm what it's intended to test when you run ember try:ember '3.2.0' I will see if I can update the tests and implementation to correspond with this.

Alternatively since I think very few people are actually using the try:ember command, maybe we just deprecate it and remove the test?

What do you think?

kategengler commented 2 years ago

That seems correct to me. Here's the minimal docs: https://github.com/ember-cli/ember-try#ember-tryember-semver-string I believe it's trying for the previous two LTSes (based on 4 releases -- it wouldn't know about 2.18).

It's used by ember observer to run tests for addons without generating configs, so I'd like to keep it.

mansona commented 1 year ago

Hey @kategengler 👋 I've finally had the opportunity to come back to this, it's been a little while 😂

So I'm doing a bit of work with @candunaj to get the ember-try CI working again and we have made the test suite mostly green on this PR: https://github.com/ember-cli/ember-try/pull/893

The remaining failing tests are all command smoke tests for the ember try:ember command and the reason they are failing is because of what I was trying to explain in my original issue. Now that I have a concrete example I will try to explain a bit better 💪

This job https://github.com/ember-cli/ember-try/actions/runs/3427921839/jobs/5711525338 is running the command ember try:ember '> 2.16.0 < 3.0.0' which I interpret to mean "please run ember-try with ember versions greater than 2.16.0 and less than 3.0.0.

Looking at the summary table it looks like it's running the following tests:

This bug that causes ember try:ember <range> to "do too much" is the reason CI is red right now. Essentially it is because any 4.x test will need some work to make it successful (with ember-auto-import@2 and webpack being added to the build)

I personally feel that the way forward is to exclude ember-canary, ember-beta, and ember-release from the config when running ember try:ember '> 2.16.0 < 3.0.0' and that will fix the bug that is causing the CI to fail. How does that sound to you?

We're working on a PR that will hopefully fix this bug but I wanted to fill you in on my thought process before we submitted it 👍

We could also investigate why 2.12.2 is being included in the config if we thought that was important but I'm not concerned because that happens to not be breaking the build 😂

kategengler commented 1 year ago

I'm curious on why 2.12.2 is included and about the duplicate. The documentation for how the range works is in the readme under versionCompatibility (which is an old feature).

I also think a possible solution would be to update the range given in the all commands test to something currently supported.

Could also add the necessary packages to those versions when they are generated over in https://github.com/ember-cli/ember-try-config

mansona commented 1 year ago

ok so looking back at the documentation I think I finally understand what you mean and I think I understand where the conceptual problems are potentially coming from.

Essentially the simple version of the fix that I have in mind would be to make versionCompatibility take precedence over what is defined in the config, but as the documentation suggests that there is a deep merge going on that would be a "breaking change" (regardless of how many people are using it)

Instead what I suggest is that we only alter the behaviour of ember try:ember to only use the provided versionCompatibility which we could do by passing an extra argument to the config generator like onlyVersionCompatibility and that would stop there being a duplicate and also stop it "doing too much". We could even make that an argument that is exposed to the ember try:ember command if you thought that was valuable? what do you think?

P.S. I was able to get CI green on #893 because of the documentation explicitly stating that you can override the allowFailure for scenarios because of the deep merge nature of it 🎉

kategengler commented 1 year ago

I don't think it is worth changing the behavior of the ember try: ember command. As you say, it is hardly used (other than by emberobserver which does rely upon the deep merging, in fact).

I do think it would be fine to test ember try:ember only with a 4.x range so that the other packages are not an issue (though I also think the eventual correct thing to do is to update ember-try-config to add those packages to those versions to match the config that would have come in a 3.28 addon).

I think using the deep merge behavior to allowFailure on the release scenario is not the right way to get CI green over on #893.

mansona commented 1 year ago

I do think it would be fine to test ember try:ember only with a 4.x range so that the other packages are not an issue

That would require the smoke-test-app to be upgraded to have the relevant dependencies and "testing against a 4.x" range would have no effect on the problem

the eventual correct thing to do is to update ember-try-config to add those packages to those versions to match the config that would have come in a 3.28 addon

I understand why this would seem like the right thing to do but unfortunately you need to know if ember-auto-import is in the dependencies or devDependencies of the addon and update the version accordingly. The reason for that is that you actually change the behaviour of the addon if you add ember-auto-import@2 to the dependencies if it wasn't there before, and if it's in the dependencies then updating the devDependencies will cause a version mismatch

kategengler commented 1 year ago

The point of ember try:ember was to have something "smart" to test against ember versions in any project without necessarily having to have the config from the blueprint, so ember-try-config provided such a config. Right now it is not serving that purpose. However, it has never been possible to successfully test all projects with this command, due to the infinite possibilities of what projects are doing -- emberobserver has always had to custom case configs for many of them, which is part of why this command deep-merges the existing config, on the theory that the project will have customized a scenario for what it needs.

So, instead of altering the try configs to allow the scenario to fail, how about altering them to add the necessary packages to those scenarios that need them? This would be using ember-try as intended and as it is for tests only we know the details of the host app.

As a separate issue, smoke-test-app does should be updated to at least a supported version of Ember.