-
Notifications
You must be signed in to change notification settings - Fork 30
Restore top level settings when loading package #1410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This is something I didn't change, but based on our observations, it might be a good idea to check it again. Are these settings properly applied to the correct level?
Command line options:
|
|
Here's one considerations for an improvement on code level. It's about the method TestCentricProject.Load(). I wonder if we should make this method static and returning a new TestCentricProject instance: I think that we have not yet clearly defined what should happen to existing settings or subpackages when the Load() method is called on an existing project. That's not an issue currently for our productive code, because the Load() method is invoked only from one code location and right after creating a new project. So, there are no existing settings or subpackages in this case. But I noticed it while writing some unit tests. I loaded a project file and assert for some settings, but failed to notice that the settings were not set from the Load() call, but instead from my arrange statements. Well, the test passed, but not exactly in the indented manner. |
CharliePoole
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this solves your issue, I'm OK with it. Let's try not to make too many changes to the design of TestCentricProject until we complete #1214. I just noticed I never labeled it to indicate it's importance and did so now. I'd like to resolve the design definitively for beta8 and implement as much of what we decide as possible. I'll add a new comment to sum up and restart the discussion.
| } | ||
|
|
||
|
|
||
| [Test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestCase attributes are sufficient.
| _view.Received().Title = "TestCentric - TestCentric.tcproj"; | ||
| } | ||
|
|
||
| [Test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
This PR fixes #1408 by restoring all top level package settings when loading a package.
The core change is located in method
TestCentricProject.Load(). It contains additional these statements now, which are responsible to apply all top level settings from the loaded package to the current (this pointer) package.Besides this change I need to slightly adapt the UI as well: the checked state of the RunAsX86 menu item, must be updated from the package settings whenever we load a project. This worked almost straight forward, besides that a reload is invoked whenever the checked state of this menu item is changed. So, I need to add some additional statements to avoid this unnecessary reload operation here.
One remark to the changes in method TestCentricPresenter.ChangePackageSettingAndReload():
I simplified that one: the 'DEFAULT' case could be removed because it's for the AgentSelection, but that is handled by the AgentSelectionController class in the meantime. Also I removed the settings.Remove() case, because we don't remove any settings in other locations, but simply applying the values. Hope, that's ok!