Refactor SonarLintSettings to follow VS threading rules #4850
Conversation
duncanp-lseg
changed the title
|
|
||
| [ImportingConstructor] | ||
| public SonarLintSettings(SVsServiceProvider serviceProvider) | ||
| : this(new ShellSettingsManager(serviceProvider)) |
There was a problem hiding this comment.
Previously the class created a concrete VS class in its constructor.
I've made two related changes:
- the code that creates the VS class has been moved to a separate class, and
- I added a
Lazyproperty to this class to delay fetching the VS service until it is required.
| if (this.writableSettingsStore != null && | ||
| !this.writableSettingsStore.CollectionExists(SettingsRoot)) | ||
| { | ||
| this.writableSettingsStore.CreateCollection(SettingsRoot); |
There was a problem hiding this comment.
This logic is moved to the new factory class.
| } | ||
|
|
||
| internal /* testing purposes */ bool GetValueOrDefault(string key, bool defaultValue) | ||
| { | ||
| try | ||
| { | ||
| return this.writableSettingsStore?.GetBoolean(SettingsRoot, key, defaultValue) | ||
| return writableSettingsStore.Value?.GetBoolean(SettingsRoot, key, defaultValue) |
There was a problem hiding this comment.
All of the references in this class to writableSettingsStore have changed to writableSettingsStore.Value. There's a unit test that checks that the factory is only invoked once.
| using SonarLint.VisualStudio.TestInfrastructure; | ||
|
|
||
| namespace SonarLint.VisualStudio.Integration.UnitTests.Settings | ||
| { | ||
| [TestClass] | ||
| public class SonarLintSettingsTests | ||
| { | ||
| private ConfigurableWritableSettingsStore settingsStore; |
There was a problem hiding this comment.
I re-wrote the existing tests to get rid of the TestInitialize, and to change the tests to be data-driven. Also, the old tests didn't use mocks, so they were more complicated than necessary. I suggest ignoring the old tests; I'll describe the behaviour of the new tests.
| this.settingsManager = new ConfigurableSettingsManager(this.settingsStore); | ||
| } | ||
| [TestMethod] | ||
| public void MefCtor_CheckIsExported() |
There was a problem hiding this comment.
New - the two standard MEF-export tests.
|
|
||
| [TestMethod] | ||
| public void IntegrationSettings_Ctor_NullArgChecks() |
There was a problem hiding this comment.
Redundant - MEF-created so they won't be null.
|
|
||
| // Assert | ||
| actual2.Should().Be(expected2, "Did not return default value"); | ||
| #region Boolean method tests |
There was a problem hiding this comment.
The interface has get and set methods for booleans, integers and strings.
There are four tests for each data type:
- GetValueOrDefault -> value is returned
- Get when store throws -> default is returned
- Get when store is null -> default is returned
- SetValue -> value is set
- SetValue when store is null -> no error
I couldn't see a simple way to de-duplicated the test code, so I copied and tweaked the tests for each data type.
| // Arrange | ||
| string expected1 = "value"; | ||
| this.settingsStore.SetString(SonarLintSettings.SettingsRoot, "key1", expected1); | ||
| #region String method tests |
There was a problem hiding this comment.
Same set of tests but for strings.
| public void GetValueOrDefault_String(string valueToReturn, string defaultValueSuppliedByCaller) | ||
| { | ||
| var store = new Mock<WritableSettingsStore>(); | ||
| store.Setup(x => x.GetString(SonarLintSettings.SettingsRoot, "aProp", defaultValueSuppliedByCaller)).Returns(valueToReturn); |
There was a problem hiding this comment.
This is effectively the only real change in the test from the boolean/int version: we're mocking a different method.
| // Arrange | ||
| int expected1 = 1; | ||
| this.settingsStore.SetInt32(SonarLintSettings.SettingsRoot, "key1", expected1); | ||
| #region Int method tests |
There was a problem hiding this comment.
And the same tests for integers.
| public class WritableSettingsStoreFactoryTests | ||
| { | ||
| [TestMethod] | ||
| public void MefCtor_CheckIsExported() |
There was a problem hiding this comment.
Two standard MEF tests
| // The MEF constructor should be free-threaded, which it will be if | ||
| // it doesn't make any external calls. | ||
| serviceProvider.Invocations.Should().BeEmpty(); | ||
| threadHandling.Invocations.Should().BeEmpty(); |
There was a problem hiding this comment.
Test the constructor is free-threaded.
|
FYI the failing tests are unrelated to these changes - flaky tests. |
|
Kudos, SonarCloud Quality Gate passed! |
* Refactor SonarLintSettings to follow VS threading rule Fixes #4843
* Refactor SonarLintSettings to follow VS threading rule Fixes #4843
Fixes #4843