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

huyphamily · 2020-02-10T01:09:51Z

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 · 2020-02-10T01:37:55Z

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%)`	⬆️
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.

rwjblue

Can you please describe the issue you are seeing in more detail (like step by step, explaining where things go wrong)? I don't fully understand what you are trying to solve, so it is somewhat hard to tell if this PR fixes it...

rwjblue · 2020-02-11T19:39:01Z

lib/dependency-manager-adapters/npm.js


-    debug('Write package.json with: \n', JSON.stringify(newPackageJSON));
+    return this._restoreOriginalDependencies().then(() => {


It seems very bizarre to restore before applying the depSet changes here.

Any cleanup/restore that was supposed to happen should have happened before the next scenario runs it's applyDependencySet.

rwjblue · 2020-02-11T19:40:14Z

lib/dependency-manager-adapters/npm.js

-          versionSeen: adapter._findCurrentVersionOf(dep),
-          packageManager: adapter.useYarnCommand ? 'yarn' : 'npm',
-        };
+    return adapter.applyDependencySet(depSet).then(() => {


We absolutely should have been entangling this all along, this fix seems good regardless of the changes within applyDependencySet itself.

rwjblue · 2020-02-11T19:41:50Z

lib/dependency-manager-adapters/workspace.js

+    let appliedDependencies = this._packageAdapters.map(adapter =>
+      adapter.applyDependencySet(depSet)
+    );
+
+    return RSVP.all(appliedDependencies).then(() => {


Like above, this seems like a great fundamental fix. Are you certain that these fixes to properly entangle applyDependencySet aren't all that is needed to solve the issue you described?

rwjblue · 2020-02-11T19:42:54Z

test/dependency-manager-adapters/npm-adapter-test.js

@@ -59,6 +59,43 @@ describe('npmAdapter', () => {
    });
  });

+  describe('#changeToDependencySet', () => {
+    it('restores the backup before changing to the new dependency set', () => {


I am fairly opposed to this 😸, setting up a new scenario to run should not itself require resetting to the backup first. This resetting seems to be masking another fundamental problem..

Yea, I agree. It does feel a bit strange to reset to the backup first, instead, it should restore the backup at the end of each scenario. Let me look into our particular use case more closely and will get back to you.

dnachev · 2020-02-13T23:11:45Z

@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 · 2020-02-14T14:59:32Z

@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 · 2020-02-15T02:58:38Z

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

rwjblue · 2020-02-15T17:41:12Z

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

ember-try/lib/dependency-manager-adapters/npm.js

Lines 145 to 158 in bff5859

    
           applyDependencySet(depSet) { 
        
             debug('Changing to dependency set: %s', JSON.stringify(depSet)); 
        
             if (!depSet) { return RSVP.resolve([]); } 
        
             let backupPackageJSON = path.join(this.cwd, this.packageJSONBackupFileName); 
        
             let packageJSONFile = path.join(this.cwd, this.packageJSON); 
        
             let packageJSON = JSON.parse(fs.readFileSync(backupPackageJSON)); 
        
             let newPackageJSON = this._packageJSONForDependencySet(packageJSON, depSet); 
        
             debug('Write package.json with: \n', JSON.stringify(newPackageJSON)); 
        
             fs.writeFileSync(packageJSONFile, JSON.stringify(newPackageJSON, null, 2)); 
        
           },

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:

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

Lines 51 to 81 in bff5859

    
           _runCommandForThisScenario(scenario) { 
        
             let task = this; 
        
             if (task._canceling) { return; } 
        
             return task.ScenarioManager.changeTo(scenario) 
        
               .then((scenarioDependencyState) => { 
        
                 if (task._canceling) { return; } 
        
                 process.env.EMBER_TRY_CURRENT_SCENARIO = scenario.name; 
        
                 task._writeHeader(`Scenario: ${scenario.name}`); 
        
                 let command = task._determineCommandFor(scenario); 
        
                 let runResults = { 
        
                   scenario: scenario.name, 
        
                   allowedToFail: !!scenario.allowedToFail, 
        
                   dependencyState: scenarioDependencyState, 
        
                   envState: scenario.env, 
        
                   command: command.join(' '), 
        
                 }; 
        
                 debug('With:\n', runResults); 
        
                 return task._runCommand({ commandArgs: command, commandOptions: task._commandOptions(scenario.env) }).then((result) => { 
        
                   if (task._canceling) { return; } 
        
                   runResults.result = result; 
        
                   task._writeFooter(`Result: ${result}`); 
        
                   return RSVP.resolve(runResults); 
        
                 }); 
        
               }); 
        
           },

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 · 2020-02-18T04:45:20Z

@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 · 2020-02-20T15:28:50Z

@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 · 2020-03-01T03:23:37Z

@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 · 2020-03-04T23:22:42Z

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

huyphamily · 2020-03-05T17:37:52Z

@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 · 2020-03-05T17:48:03Z

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

rwjblue · 2020-03-08T22:23:34Z

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

huyphamily · 2020-03-09T23:05:51Z

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

rwjblue · 2020-03-10T19:36:06Z

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).

ember-try/lib/dependency-manager-adapters/npm.js

Lines 145 to 158 in bff5859

    
           applyDependencySet(depSet) { 
        
             debug('Changing to dependency set: %s', JSON.stringify(depSet)); 
        
             if (!depSet) { return RSVP.resolve([]); } 
        
             let backupPackageJSON = path.join(this.cwd, this.packageJSONBackupFileName); 
        
             let packageJSONFile = path.join(this.cwd, this.packageJSON); 
        
             let packageJSON = JSON.parse(fs.readFileSync(backupPackageJSON)); 
        
             let newPackageJSON = this._packageJSONForDependencySet(packageJSON, depSet); 
        
             debug('Write package.json with: \n', JSON.stringify(newPackageJSON)); 
        
             fs.writeFileSync(packageJSONFile, JSON.stringify(newPackageJSON, null, 2)); 
        
           },

rwjblue · 2020-05-23T14:42:32Z

@huyphamily - Friendly ping 😃😃

huyphamily · 2020-06-10T09:35:37Z

@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 · 2020-07-10T19:16:35Z

@rwjblue any thoughts on this?

rwjblue · 2020-07-13T16:37:41Z

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.

rwjblue

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):

ember-try/lib/dependency-manager-adapters/bower.js

Lines 93 to 110 in aa4e3ff

    
           cleanup() { 
        
             let adapter = this; 
        
             if (this.hasBowerFileInitially) { 
        
               return adapter._restoreOriginalBowerFile().then(() => { 
        
                 debug('Remove backup bower.json'); 
        
                 return rimraf(path.join(adapter.cwd, adapter.bowerJSONBackupFileName)); 
        
               }).catch((e) => { 
        
                 console.log('Error cleaning up bower scenario:', e); // eslint-disable-line no-console 
        
               }) 
        
                 .then(() => { 
        
                   return adapter._install(); 
        
                 }); 
        
             } else { 
        
               return rimraf(path.join(adapter.cwd, adapter.bowerJSONFileName)).then(() => { 
        
                 return rimraf(path.join(adapter.cwd, 'bower_components')); 
        
               }); 
        
             } 
        
           },

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

ember-try/lib/dependency-manager-adapters/workspace.js

Lines 88 to 90 in aa4e3ff

    
           cleanup() { 
        
             return RSVP.all(this._packageAdapters.map(adapter => adapter.cleanup())); 
        
           },

huyphamily · 2020-09-01T19:17:15Z

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):

ember-try/lib/dependency-manager-adapters/bower.js

Lines 93 to 110 in aa4e3ff

cleanup() {

let adapter = this;

if (this.hasBowerFileInitially) {

return adapter._restoreOriginalBowerFile().then(() => {

debug('Remove backup bower.json');

return rimraf(path.join(adapter.cwd, adapter.bowerJSONBackupFileName));

}).catch((e) => {

console.log('Error cleaning up bower scenario:', e); // eslint-disable-line no-console

})

.then(() => {

return adapter._install();

});

} else {

return rimraf(path.join(adapter.cwd, adapter.bowerJSONFileName)).then(() => {

return rimraf(path.join(adapter.cwd, 'bower_components'));

});

}

},

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

ember-try/lib/dependency-manager-adapters/workspace.js

Lines 88 to 90 in aa4e3ff

cleanup() {

return RSVP.all(this._packageAdapters.map(adapter => adapter.cleanup()));

},

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 · 2020-09-22T15:36:28Z

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...

…cenarios. When there is only a single scenario to run (e.g. `ember try:one`) we should not be cleaning up as part of running the scenario itself because in that case we would immediately cleanup _anyways_. If we forced cleanup to happen during the scenario run, then it would be impossible to have `--skip-cleanup` work at all.

rwjblue · 2020-11-02T16:05:04Z

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 Dependencies can leak between scenarios #579)
using a single scenario with --skip-cleanup, confirm that cleanup is not actually done

bertdeblock · 2024-12-12T14:27:05Z

Closing for now.

huyphamily requested review from kategengler and rwjblue February 10, 2020 01:46

rwjblue requested changes Feb 11, 2020

View reviewed changes

huyphamily force-pushed the backup branch from c2d042f to f1ab3e3 Compare March 1, 2020 03:21

huyphamily requested a review from rwjblue March 1, 2020 03:24

rwjblue requested changes Jul 13, 2020

View reviewed changes

huyphamily force-pushed the backup branch from f1ab3e3 to e72d29e Compare September 1, 2020 19:14

kategengler mentioned this pull request Oct 21, 2020

Dependencies can leak between scenarios #579

Open

rwjblue force-pushed the backup branch from e72d29e to 7764b35 Compare November 2, 2020 15:43

Restore backup files after each scenario

31bfb84

rwjblue force-pushed the backup branch from 7764b35 to 48ea3c8 Compare November 2, 2020 15:45

Ensure the stub dependency adapter conforms to the interface.

5549aba

bertdeblock closed this Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

huyphamily commented Feb 10, 2020 •

edited

Loading

codecov bot commented Feb 10, 2020 •

edited

Loading

rwjblue left a comment

rwjblue Feb 11, 2020

rwjblue Feb 11, 2020

rwjblue Feb 11, 2020

rwjblue Feb 11, 2020

huyphamily Feb 12, 2020

dnachev commented Feb 13, 2020

rwjblue commented Feb 14, 2020

huyphamily commented Feb 15, 2020

rwjblue commented Feb 15, 2020

huyphamily commented Feb 18, 2020 •

edited

Loading

rwjblue commented Feb 20, 2020

huyphamily commented Mar 1, 2020

rwjblue commented Mar 4, 2020

huyphamily commented Mar 5, 2020

huyphamily commented Mar 5, 2020

rwjblue commented Mar 8, 2020

huyphamily commented Mar 9, 2020

rwjblue commented Mar 10, 2020

rwjblue commented May 23, 2020

huyphamily commented Jun 10, 2020 •

edited

Loading

huyphamily commented Jul 10, 2020

rwjblue commented Jul 13, 2020

rwjblue left a comment

huyphamily commented Sep 1, 2020

rwjblue commented Sep 22, 2020

rwjblue commented Nov 2, 2020

bertdeblock commented Dec 12, 2024


		debug('Write package.json with: \n', JSON.stringify(newPackageJSON));
		return this._restoreOriginalDependencies().then(() => {

	cleanup() {
	let adapter = this;
	if (this.hasBowerFileInitially) {
	return adapter._restoreOriginalBowerFile().then(() => {
	debug('Remove backup bower.json');
	return rimraf(path.join(adapter.cwd, adapter.bowerJSONBackupFileName));
	}).catch((e) => {
	console.log('Error cleaning up bower scenario:', e); // eslint-disable-line no-console
	})
	.then(() => {
	return adapter._install();
	});
	} else {
	return rimraf(path.join(adapter.cwd, adapter.bowerJSONFileName)).then(() => {
	return rimraf(path.join(adapter.cwd, 'bower_components'));
	});
	}
	},

	cleanup() {
	return RSVP.all(this._packageAdapters.map(adapter => adapter.cleanup()));
	},

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

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

Conversation

huyphamily commented Feb 10, 2020 • edited Loading

codecov bot commented Feb 10, 2020 • edited Loading

Codecov Report

rwjblue left a comment

Choose a reason for hiding this comment

rwjblue Feb 11, 2020

Choose a reason for hiding this comment

rwjblue Feb 11, 2020

Choose a reason for hiding this comment

rwjblue Feb 11, 2020

Choose a reason for hiding this comment

rwjblue Feb 11, 2020

Choose a reason for hiding this comment

huyphamily Feb 12, 2020

Choose a reason for hiding this comment

dnachev commented Feb 13, 2020

rwjblue commented Feb 14, 2020

huyphamily commented Feb 15, 2020

rwjblue commented Feb 15, 2020

huyphamily commented Feb 18, 2020 • edited Loading

rwjblue commented Feb 20, 2020

huyphamily commented Mar 1, 2020

rwjblue commented Mar 4, 2020

huyphamily commented Mar 5, 2020

huyphamily commented Mar 5, 2020

rwjblue commented Mar 8, 2020

huyphamily commented Mar 9, 2020

rwjblue commented Mar 10, 2020

rwjblue commented May 23, 2020

huyphamily commented Jun 10, 2020 • edited Loading

huyphamily commented Jul 10, 2020

rwjblue commented Jul 13, 2020

rwjblue left a comment

Choose a reason for hiding this comment

huyphamily commented Sep 1, 2020

rwjblue commented Sep 22, 2020

rwjblue commented Nov 2, 2020

bertdeblock commented Dec 12, 2024

huyphamily commented Feb 10, 2020 •

edited

Loading

codecov bot commented Feb 10, 2020 •

edited

Loading

huyphamily commented Feb 18, 2020 •

edited

Loading

huyphamily commented Jun 10, 2020 •

edited

Loading