Reorganize settings to use common code but separate prefixes #396
Conversation
CharliePoole
changed the title
| @@ -66,7 +65,7 @@ public override void OnTestLoaded(TestNode testNode) | |||
|
|
|||
| _view.Tree.Add(treeNode); | |||
|
|
|||
| SetInitialExpansion(displayStyle, treeNode); | |||
| SetInitialExpansion((TreeDisplayStyle)displayStyle, treeNode); | |||
There was a problem hiding this comment.
It is a bit confusing that in testcentric.gui Gui.TestTree.InitialTreeDisplay contains a DisplayStyle and in the experimental gui is contains a TreeDisplayStyle, and these enums have the same values (with the same names).
There was a problem hiding this comment.
You're right. I'll take another look.
Generally, I resisted making any changes that were "incidental" to the big change I was making, i.e. unifying the handling of settings. In this case, I kept the different definitions because I thought the experimental value might have more things added in the future and because the knowledge of the different ways of expanding the tree don't belong in the model. However, the name is rather bad, since it doesn't contain the word "expansion," which is what this setting is all about.
There was a problem hiding this comment.
Changed both to InitialTreeExpansion
| public void GetSettingWithDefault_WhenPresent_ReturnsValue(string name, object expected) | ||
| { | ||
| _settings.SaveSetting(name, expected); | ||
| object actual = _settings.GetSetting(name); |
There was a problem hiding this comment.
We are not "getting" the setting with default, so the name of the test is a bit misleading.
There was a problem hiding this comment.
Good catch! The test is not testing what it's supposed to test.
There was a problem hiding this comment.
OK! So reviewing this led me to reconsider the entire structure of SettingsGroupTests. It relied on Fakes.SettingsService duplicating the semantics of the engine's SettingsService. That doesn't really make sense to me now that I look at it. It's too much effort to keep the fake up to date. I'm converting to use of a mock instead. Once I push the commit, I'd appreciate your taking another look at these tests.
| public void WhenSettingIsNotValid_DefaultSettingIsReturned() | ||
| { | ||
| _settings.SaveSetting( "X", "1y25" ); | ||
| Assert.That(_settings.GetSetting( "X", 42 ), Is.EqualTo(42)); |
There was a problem hiding this comment.
There are some spaces before/after the parenthesis in this method. Also I had to think about why 1y25 was invalid. It was only when I looked at the code in VS I noticed that 42 forces the returned value to be an integer. I'm not sure if we could make this more clear by changing GetSetting( to GetSetting<int>( even though the type parameter is redundant.
There was a problem hiding this comment.
Rather than the redundant Type parameter, I changed the name of the test and added a comment.
Fixes #315