Fix resetting proposal settings #889

dgdavid · 2019-04-03T07:29:58Z

Problem

Initial settings are not being restored before each proposal attempt because

Yast.deep_copy is quite a fail that only copies one level, leaving the second level as references instead of copies. — extracted from Ancor's comment

Solution

~~Overwrite the ProposalSettings#dup~~ Added a ProposalSettings#deep_copy method to make a copy based on a Marshal serialization instead of use Yast.deep_copy because

It did its job with the YCP -> Ruby translation, but it should never be used in new code because it does not do what it promises. We should use Ruby mechanisms for performing deep copies, such as Marshall. — extracted from Ancor's comment

Testing

Added a new unit tests
Fixed some existing tests

test/y2storage/initial_guided_proposal_test.rb

test/y2storage/proposal_settings_test.rb

src/lib/y2storage/proposal_settings.rb

src/lib/y2storage/initial_guided_proposal.rb

test/y2storage/proposal/settings_generator/ng_test.rb

test/y2storage/proposal_settings_test.rb

package/yast2-storage-ng.changes

test/y2storage/proposal_settings_test.rb

test/y2storage/proposal/settings_generator/ng_test.rb

lslezak

Looks OK, but I'd leave the final approval on @ancorgs 😉

ancorgs

I agree the #to_s based tests may look a bit weird, but I don't think it makes the situation any worse than it was with those tests. So LGTM

lslezak · 2019-04-03T15:46:22Z

Yes, looks better to me.

src/lib/y2storage/proposal_settings.rb

test/y2storage/proposal_settings_test.rb

joseivanlopez · 2019-04-03T16:34:51Z

I see some tests are starting to fail because "singleton can't be dumped". I suspect this is happening because "double" objects cannot be marshaled. Maybe we can fix it by replacing double by instance_double, for example here https://github.com/yast/yast-storage-ng/blob/master/test/y2storage/dialogs/guided_setup/select_filesystem/ng_test.rb#L38

Or even better, we can avoid doubles at all, and simply create "real" VolumeSpecification objects.

dgdavid · 2019-04-04T06:52:44Z

@joseivanlopez

Thank you for your feedback.

I already updated the PR with your suggestions, but I fix those tests that are failing. Using instance_double do not fix them.

joseivanlopez · 2019-04-04T07:45:16Z

@dgdavid I was not sure about that solution (I was only guessing). First of all, we have to be sure that the problem is the combination of Marshall + mocking. This can be easily checked. And if we confirm that, then we have 3 options:

a) try to use real VolumeSpecification objects in tests, although I don't like it because we force to always use real settings.
b) do not redefine ProposalSettings#==. The problem I see with this is that settings == settings.dup but settings != settings.deep_copy.
c) do not use Marshal to redefine ProposalSettings#==, but maybe a good solution would require too much effort.

So, if finally the problem is Marshal + mocking, IMHO I would go with option b) iff settings != settings.deep_copy is not a problem (not sure if this could affect in some parts of the code).

@ancorgs How do you see it?

ancorgs · 2019-04-04T10:46:41Z

I wouldn't redefine ProposalSettings#== just to have nicer tests. That's a change with potentially many implications, definitely not something to do in the SLE-15-SP1 branch at this point in time.

Fixing the deep dup is the further I would go.

dgdavid · 2019-04-04T10:53:35Z

@joseivanlopez, @ancorgs

So, I'll undo the ~~latest changes and keep the PR as it was 20 hours ago.~~ ProposalSettings#== redefinition. — (edited after a chat with @joseivanlopez ;)).

Thank you!

Based on a Marshal (de)serialization.

The default #dup creates a shallow and we were changing the semantic. Renaming it to a `deep_copy` is more consistent with its behavior.

Co-Authored-By: David Díaz González <dgonzalez@suse.de>

After changes done in SettingsGenerator, InitialGuidedProposal does not need to copy the original settings. Keeping a reference to them is enough.

Based on suggestions from code review Co-Authored-By: José Iván López González <jlopez@suse.com>

joseivanlopez

LGTM

coveralls · 2019-04-04T12:03:12Z

Coverage increased (+0.0003%) to 97.378% when pulling cdfced4 on feature/bsc-1130392-fix-reset-proposal-settings into 5518494 on SLE-15-SP1.

dgdavid requested review from joseivanlopez and ancorgs April 3, 2019 07:29

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch from 6c981bf to 15d4fb0 Compare April 3, 2019 07:33

dgdavid changed the title ~~Feature/bsc 1130392 fix reset proposal settings~~ [WIP] Fix reset proposal settings Apr 3, 2019

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch 2 times, most recently from a92c6b3 to fdb2724 Compare April 3, 2019 08:56

dgdavid changed the title ~~[WIP] Fix reset proposal settings~~ Fix reseting proposal settings Apr 3, 2019

dgdavid changed the title ~~Fix reseting proposal settings~~ Fix resetting proposal settings Apr 3, 2019

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch from fdb2724 to 00bc177 Compare April 3, 2019 09:21

ancorgs reviewed Apr 3, 2019

View reviewed changes

test/y2storage/initial_guided_proposal_test.rb Outdated Show resolved Hide resolved

ancorgs reviewed Apr 3, 2019

View reviewed changes

test/y2storage/initial_guided_proposal_test.rb Outdated Show resolved Hide resolved

ancorgs reviewed Apr 3, 2019

View reviewed changes

test/y2storage/proposal_settings_test.rb Outdated Show resolved Hide resolved

ancorgs reviewed Apr 3, 2019

View reviewed changes

test/y2storage/proposal_settings_test.rb Outdated Show resolved Hide resolved

ancorgs reviewed Apr 3, 2019

View reviewed changes

test/y2storage/proposal_settings_test.rb Outdated Show resolved Hide resolved

lslezak reviewed Apr 3, 2019

View reviewed changes

src/lib/y2storage/proposal_settings.rb Outdated Show resolved Hide resolved

src/lib/y2storage/initial_guided_proposal.rb Outdated Show resolved Hide resolved

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch from ebe3607 to 89d70fb Compare April 3, 2019 10:15

dgdavid commented Apr 3, 2019

View reviewed changes

test/y2storage/proposal/settings_generator/ng_test.rb Outdated Show resolved Hide resolved

ancorgs reviewed Apr 3, 2019

View reviewed changes

test/y2storage/proposal_settings_test.rb Outdated Show resolved Hide resolved

lslezak reviewed Apr 3, 2019

View reviewed changes

package/yast2-storage-ng.changes Outdated Show resolved Hide resolved

test/y2storage/proposal_settings_test.rb Outdated Show resolved Hide resolved

test/y2storage/proposal/settings_generator/ng_test.rb Outdated Show resolved Hide resolved

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch from 89a93ce to 54987c0 Compare April 3, 2019 14:30

lslezak reviewed Apr 3, 2019

View reviewed changes

ancorgs approved these changes Apr 3, 2019

View reviewed changes

joseivanlopez reviewed Apr 3, 2019

View reviewed changes

src/lib/y2storage/proposal_settings.rb Outdated Show resolved Hide resolved

test/y2storage/proposal_settings_test.rb Outdated Show resolved Hide resolved

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch from a75490e to aa372fe Compare April 4, 2019 06:52

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch 2 times, most recently from 8adfcd3 to 3a01573 Compare April 4, 2019 11:45

dgdavid and others added 10 commits April 4, 2019 12:53

Overwrite ProposalSettings#dup to produce a deep copy

70dddd2

Based on a Marshal (de)serialization.

Reset the settings properly between proposal attempts

8ca1716

Update version and changelog

0b59e1f

Update from code review

ab38070

Rename ProposalSettings#dup to ProposalSettings#deep_copy

1bd2e5d

The default #dup creates a shallow and we were changing the semantic. Renaming it to a `deep_copy` is more consistent with its behavior.

Use ProposalSettings#deep_copy also in SettingsGenerator

017fea8

Update from code review

6d0e66a

Co-Authored-By: David Díaz González <dgonzalez@suse.de>

Remove unnecessary settings deep copies

d193734

After changes done in SettingsGenerator, InitialGuidedProposal does not need to copy the original settings. Keeping a reference to them is enough.

Update settings generator tests

fc9f730

Update some unit tests

cdfced4

Based on suggestions from code review Co-Authored-By: José Iván López González <jlopez@suse.com>

dgdavid force-pushed the feature/bsc-1130392-fix-reset-proposal-settings branch from 3a01573 to cdfced4 Compare April 4, 2019 11:56

joseivanlopez approved these changes Apr 4, 2019

View reviewed changes

dgdavid merged commit 042eef9 into SLE-15-SP1 Apr 4, 2019

dgdavid deleted the feature/bsc-1130392-fix-reset-proposal-settings branch April 4, 2019 12:03

dgdavid mentioned this pull request Apr 4, 2019

Merge SLE-15-SP1 #893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resetting proposal settings #889

Fix resetting proposal settings #889

dgdavid commented Apr 3, 2019 •

edited

lslezak left a comment

ancorgs left a comment

lslezak commented Apr 3, 2019

joseivanlopez commented Apr 3, 2019 •

edited

dgdavid commented Apr 4, 2019

joseivanlopez commented Apr 4, 2019

ancorgs commented Apr 4, 2019

dgdavid commented Apr 4, 2019 •

edited

joseivanlopez left a comment

coveralls commented Apr 4, 2019

Fix resetting proposal settings #889

Fix resetting proposal settings #889

Conversation

dgdavid commented Apr 3, 2019 • edited

Problem

Solution

Testing

lslezak left a comment

Choose a reason for hiding this comment

ancorgs left a comment

Choose a reason for hiding this comment

lslezak commented Apr 3, 2019

joseivanlopez commented Apr 3, 2019 • edited

dgdavid commented Apr 4, 2019

joseivanlopez commented Apr 4, 2019

ancorgs commented Apr 4, 2019

dgdavid commented Apr 4, 2019 • edited

joseivanlopez left a comment

Choose a reason for hiding this comment

coveralls commented Apr 4, 2019

dgdavid commented Apr 3, 2019 •

edited

joseivanlopez commented Apr 3, 2019 •

edited

dgdavid commented Apr 4, 2019 •

edited