Restore backup files before changing to new dependency set for new scenarios.

[huyphamily]

This PR is created to fix an issue we are having when using buildManagerOptions and removing the default --no-lockfile option in order to use our yarn.lock file. In the current behavior, when switching between scenarios, versions from the previous scenario gets carried over to the next scenario, which is not the ideal behavior that we want. We want to reset the yarn.lock to the original version, before changing the dependencies of the new scenario.

In order to accomplish this, we will restore all backups before installing new dependencies for the new scenario. This will not affect the current default behavior for those with default settings, but will greatly improve the experience of those who are using buildManagerOptions to override the default --no-lockfile.

[codecov[bot]]

Codecov Report

Merging #451 into master will decrease coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
- Coverage   96.91%   96.90%   -0.02%     
==========================================
  Files          17       17              
  Lines         584      581       -3     
==========================================
- Hits          566      563       -3     
  Misses         18       18              

Impacted Files	Coverage Δ
lib/dependency-manager-adapters/npm.js	100.00% <100.00%> (ø)
lib/tasks/try-each.js	84.70% <100.00%> (ø)
lib/utils/scenario-manager.js	95.45% <100.00%> (+1.01%)	:arrow_up:
lib/dependency-manager-adapters/workspace.js	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cd7a913...5549aba. Read the comment docs.

[dnachev]

@rwjblue The issue is the following:

A project was using [email protected] (as observed in yarn.lock). The tests were passing correct without ember-try, but the scenario, which doesn't touch the package.json failed in ember-try.

package.json has devDependency: "ember-data": "~3.8.0" yarn.lock has [email protected]

And the ember-try scenario was: 3.4 -> "ember-data": "~3.4.0" current -> no changes

• The 3.4 scenario runs and replaces ember-data in yarn.lock with 3.4.1. It passes.
• The next scenario is "current", which asks for no changes in package.json. Original package.json is restored and yarn install is ran. It detects that yarn.lock is not valid anymore and re-resolves ember-data package again, which ends up picking up 3.8.3, which is the current 3.8.x version.
• The test fails, because there is a bug in 3.8.3, which breaks the application.

The ask is to make sure that ember-try doesn't introduce additional variability in the tests in case lockfile is used by ember-try, even though these failures might be valid.

[rwjblue]

@dnachev - This PR (at the moment) is resetting the various files before starting a scenario, when instead it should be ensuring that the resetting happens when the current scenario is completed.

Specifically, in your timeline:

• The 3.4 scenario runs and replaces ember-data in yarn.lock with 3.4.1. It passes.

Once the scenario passes, the package.json and yarn.lock should be reset to their original values.

• The next scenario is "current", which asks for no changes in package.json. Original package.json is restored and yarn install is ran. It detects that yarn.lock is not valid anymore and re-resolves ember-data package again, which ends up picking up 3.8.3, which is the current 3.8.x version.

The original package.json should not restored by running the next ember-try scenario (which is what you are inferring here), it should be restored at the end of the 3.4 scenario.

[huyphamily]

@rwjblue yea, I agree. I will refactor the code and move the logic for backup into the try-each.

[rwjblue]

I did a bit more digging, and I think the issue is here:

https://github.com/ember-cli/ember-try/blob/bff58592fc44e25e19e534418cf6ef5227763cd8/lib/dependency-manager-adapters/npm.js#L145-L158

As you can see, the original package.json contents are always used as the starting point before merging the scenario's specific changes into it. This means that the package.json is effectively being reset upon setup of each scenario. However, we do nothing RE: the lockfiles there.

When using try:one this discrepancy doesn't really matter too much, because we are only ever running a single scenario which ends with either --skip-cleanup or properly restoring the backup files (including lock files), but when using try:each those backup files are only restored at the end of all scenarios.

IMHO, the best fix would be to update here:

https://github.com/ember-cli/ember-try/blob/bff58592fc44e25e19e534418cf6ef5227763cd8/lib/tasks/try-each.js#L51-L81

To ensure that we run this.ScenarioManager.cleanup() after the command has been run.

We should also deprecate the --skip-cleanup option when using try:each, I don't think it really provides any value (it is pretty hard to reason about what will be the "final state" when the process exits.

[huyphamily]

@rwjblue Yea, I don't have enough context to understand why --skip-cleanup was there to begin with, so I rather not touch it. I also don't think we should call cleanup after every scenario in the try each because cleanup involves restoring the backup to the original files (package.json.ember-try -> package.json) and then deleting the backup (package.json.ember-try). Deleting the backup after each scenario is unnecessary, as you only really need to do it at the very end, and adds one more unnecessary step between scenarios because now you have to back up the original dependencies again since you deleted the backup.

I think the easiest thing to do is to call _restoreOriginalDependencies when each task finishes inside the _runCommandForThisScenario function in try-each.js:

return task._runCommand({ commandArgs: command, commandOptions: task._commandOptions(scenario.env) }).then((result) => {
  if (task._canceling) { return; }

  runResults.result = result;
  task._writeFooter(`Result: ${result}`);
  // add our change here
  return task._restoreOriginalDependencies().then(() => RSVP.resolve(runResults));
});

This is a much easier change and won't affect current behavior. Let me know what you think.

edit: minor edit to the code so that runResults gets return last. You could even remove the RSVP.resolve as I don't think that really adds anything since you're returning a promise no matter what.

[rwjblue]

@huyphamily In general that seems fine (only restoring the package.json and lockfiles), but we need to go through the ScenarioManager API and not be calling private methods on the underlying adapters directly. You'll need to see if there is an API on ScenarioManager to do this, if not add one.

[huyphamily]

@rwjblue Code updated. I have added an API to ScenarioManger for restoring the original backup and calling it at the end of each scenario.

[rwjblue]

I thought you mentioned that you didn't want to restore the node_modules/ directory in addition to restoring package.json/yarn.lock?

[huyphamily]

@rwjblue yea, you're right, we don't want to restore the node_modules/... Should I separate the backup of every file except the node_modules out into another method which will be called in restoreOriginalDependencies and keep restoreOriginalDependencies as a private method? The new method will not be private.

[huyphamily]

@rwjblue or we can pass a parameter into restoreOriginalDependencies which determines whether to backup node_modules or not. What are your thoughts?

[rwjblue]

Hmm, I thought it we already had a method that restored the manifest and lockfiles but not the fully node_modules/. :thinking:

[huyphamily]

@rwjblue we do not...unless I miss something?

[rwjblue]

I guess I was assuming that the fix here would be to modify applyDependencySet to restore the associated lockfile in addition to the package.json (we currently restore the original package.json because we start with the backup package.json every time).

https://github.com/ember-cli/ember-try/blob/bff58592fc44e25e19e534418cf6ef5227763cd8/lib/dependency-manager-adapters/npm.js#L145-L158

[rwjblue]

@huyphamily - Friendly ping 😃😃

[huyphamily]

@rwjblue Apologies for the super late replies.

For try:one, we properly restore the backup files, so we can ignore this case.

For try:each, the current behavior is to restore the backup files at the end of all scenarios, which is where our issue lies. We want to restore at the end of each scenario, hence why the change to try-each.js.

We do not want to restore any of the associated backup files in applyDependencySet because of your earlier statement, which makes complete sense: This PR (at the moment) is resetting the various files before starting a scenario, when instead it should be ensuring that the resetting happens when the current scenario is completed.

This change will restore the backup files at the end of each scenario in try:each

[huyphamily]

@rwjblue any thoughts on this?

[rwjblue]

For try:each, the current behavior is to restore the backup files at the end of all scenarios, which is where our issue lies. We want to restore at the end of each scenario, hence why the change to try-each.js.

The thing is, I don't really want to restore the node_modules folder and the various individual files (e.g. yarn.lock, shrinkwrap, etc). However, I think that particular thing could be something we change in the future (I'll make an issue for it) and doesn't need to be coupled to this PR.

[huyphamily]

There are still a few changes needed here to ensure that all of the built in managers work properly with the main change.

• Update the bower adapter to implement restoreOriginalDependencies (would need to be extracted from current cleanup logic):

https://github.com/ember-cli/ember-try/blob/aa4e3fff85b538f3b1aa77b00af0add6bc7d6f94/lib/dependency-manager-adapters/bower.js#L93-L110

• Update workspace manager to implement restoreOriginalDependencies, this will look basically the same as the cleanup method (but for restoreOriginalDependencies):

https://github.com/ember-cli/ember-try/blob/aa4e3fff85b538f3b1aa77b00af0add6bc7d6f94/lib/dependency-manager-adapters/workspace.js#L88-L90

I just got back from paternity leave and things got a little hectic, so sorry for the late reply.

I made the changes to add restoreOriginalDependencies in workplace adapter. However, I did not add restoreOriginalDependencies for bower since you mention in the past that not to add to bower as it's going away. Is this not still true?

[rwjblue]

However, I did not add restoreOriginalDependencies for bower since you mention in the past that not to add to bower as it's going away. Is this not still true?

Ya, it is true in general, but without doing something here we are going to throw an error if you have Bower scenarios and that is not ok...

[rwjblue]

I've rebased and made a tweak to ensure that when running ember try:one <scenario> --skip-cleanup we don't actually cleanup.

I'll be working on finalizing this PR today, I think at this point we just need lots of tests:

• using multiple scenarios, confirm that the package.json's are not gradually mutated after each (reported here and in #579)
• using a single scenario with --skip-cleanup, confirm that cleanup is not actually done