Fix applying package settings

[rowo360]

This PR fixes #1407. Although the issue description sounds like a UI update issue, it turned out that the package settings were not properly applied.

The solution consist of two important parts:

• Usage of PackageSetting objects instead of <string, object> pairs to apply a setting
• Use of method TestPackage.AddSetting(TestPackage) instead of method Settings.Set(key, value)

The first part helps to avoid boxing of value types - so that a bool value if properly set as a bool type and not of type object(bool).
The second part is necessary so that that a setting is not only applied as a top-level setting, but applied also to all subpackages.
The package setting sub view, in turn, requests the values directly from the subpackages, so it will display the changed values right away.

Some further details are described in the next comments.

[rowo360]

So far the setting dialog stores a dictionary <string, object> for the package settings. Now we use a list of PackageSetting objects instead to prevent boxing of value types. The PackageSetting objects are created explicitly in method ApplySettings() of class AdvancedLoaderSettingsPage while binding the UI state back to the setting objects.

[rowo360]

One benefit of using PackSetting objects is that it allows us to call TestPackage.AddSetting(TestPackage). This method applies a setting to all subpackages, so we don't need to perform this task on our own. However, please notice the new keyword - additional to the implementation in class TestPackage we need to set the IsDirty flag.

public new void AddSetting(PackageSetting setting)

[rowo360]

I did one additional code clean-up:
When a TestCentricProject is created, we pass our command line options to the NUnit.Engine. This applies also for test parameters. Here. we add the test parameters as setting TestParametersDictionary and TestParameters. I decided to remove the TestParameters part as this setting is outdated in favor of TestParametersDictionary. And we already don't use setting TestParameters when using the setting dialog. Is this Ok?

I spotted also the two command line options: SimulateUnloadError and SimulateUnloadTimeout in the same TestCentricProject constructor. Are these still supported? I scanned through the entire TestCentric and NUnit code base, but didn't find any matches. If they are unused and left over from the past, I can also remove them.

And one last remark: we could rename the existing method TestCentricProject.AddSetting(...) to SetTopLevelSetting(...). And maybe use also a PackageSetting as parameter instead of (string, object)? Just to avoid this confusion in future.

public void AddSetting(string key, object value)
{
    Settings.Set(key, value);
    IsDirty = true;
}

[CharliePoole]

See inline comment regarding AddSettings usage.

[CharliePoole]

Use of AddSettings here suggests that we have information that any setting included in the dialog pages is intended to be set on every subpackage. But we have no guarantee that this is true and even if it is true today, new settings may not meet this criterion.

After going through the code carefully, I can see that this is not a change you introduced but that the code always made this assumption.

[CharliePoole]

Looking through the settings, my conclusions are as follows...

General Tab - all settings should be top level only. That is, they only appear on the test project itself since they only relate to the GUI.

Tree Display Tab - also all top level only.

Assembly Load Settings Tab - These may be removed as we discussed, but while we have them they should work correctly. These settings differ. Specificallty...

• Agent Limit has to be top level as our algorithm does not allow separate limits per subpackage.
• Shadow Copy and Principal policy could vary by assembly in theory, but we don't support that. However, the values currently need to be promulgated to each assembly so that the agent executing tests can see them.

Automatic Reload Tab - Top Level Only

Project Editor Tab - Top Level Only

So, in summary, most settings are top-level only. Could we keep two lists, one for top-level settings and one for per-assembly settings?

[CharliePoole]

Regarding some of your questions...

1. TestParameters is still there, just used by old versions of NUnit Framework. BTW, we will need to do something about soon about serializing TestParametersDictionary as XML, as we did in our own engine code. Probably should be done in NUnit though.
2. The two "Simulate" settings were used in our tests. You can do a search to see if there are commented out tests for this. I believe our own engine or engine tests had code for this.
3. I'd rather see us drop the new AddSetting and have two private methods, one to Add to the top level only and another to also promulgate the setting downwards by calling AddSetting. Both would set IsDirty.

Let me know if I missed anything, but everything looks great if we can distinguish the two sorts of settngs in our behavior. FWIW, I don't think the user needs to know whether we promulgate the settings so long as it all works.

[rowo360]

Good suggestions - I've incorporated the code review comments:

• The settings dialog contains two lists now:
  TopLevelPackageSettingChanges and SubPackageSettingChanges
  The individual settings pages can add their settings to either of these lists.
• The TestCentricProject class has two methods to apply a setting:
  SetTopLevelSetting(PackageSetting setting) and SetSubPackageSetting(PackageSetting setting)
  Depending on the individual setting the appropriate method must be invoked.

[rowo360]

And regarding these two "simulate-unload..." settings, I finally spotted their usage in our TestCentric.Agent.Core code base. But since we don't use that one anymore and these settings are also not used in the NUnit code base, I decided to clean it up and remove them.

[rowo360]

I'm going to merge this PR now, so that I can continue with the related task (#1408). Nonetheless I can add additional changes afterwards if requested.