Skip to content

Update refresh scenario tests - #205

Merged
Avani Gupta (avanigupta) merged 2 commits into
mainfrom
avanigupta/updaterefreshtests
Oct 16, 2020
Merged

Avani Gupta (avanigupta) merged 2 commits into
mainfrom
avanigupta/updaterefreshtests

Conversation

@avanigupta

Copy link
Copy Markdown
Member

I found that some refresh scenario tests were actually not testing the intended functionality because of the following reasons:

  • We were selecting/registering KVs based on just key name, whereas all the KVs in mock data also had a label. That led to unintentionally registering/selecting non-existent keys with null labels, and hence the test took a different code path than expected.
    For Eg:

    • When RefreshAsync call triggers RefreshIndividualKeyValues, instead of calling GetKeyValueChange and processing the "changes", we load the setting again with GetConfigurationSettingAsync. That misses the intention of testing the GetKeyValueChange method.

    • When LoadKeyValuesRegisteredForRefresh is called, we should skip loading KV that have already loaded as part of Select clause. But since we registered a KV without its label, it's treated as a new KV and loaded again.

  • We were always returning the entire mock data collection of KV, regardless of what is present in the Select clause. This led to unintentional behavior in tests like RefreshTests_RefreshRegisteredKeysAreLoadedOnStartup_DefaultUseQuery and RefreshTests_RefreshRegisteredKeysAreLoadedOnStartup_CustomUseQuery. These tests would always pass regardless of ConfigureRefresh because the Select clause (or a lack thereof), ensures that all KVs from mock data collection are loaded.

To fix this, I have made the following changes to these tests:

  1. I've explicitly added labels to every key that is registered for refresh.
  2. Because of # 1, I also had to register a new mock client setup for GetConfigurationSettingAsync(ConfigurationSetting setting, bool cond, CancellationToken ct) method in GetMockConfigurationClient().
  3. Because of # 2, I was able to refactor all the tests that had the same mock client setup and replace the duplicated code with the shared GetMockConfigurationClient() method.

Originally posted by Avani Gupta (@avanigupta) in #204 (comment)

Comment thread tests/Tests.AzureAppConfiguration/RefreshTests.cs Outdated
Comment thread tests/Tests.AzureAppConfiguration/RefreshTests.cs Outdated
@abhilasharora

Copy link
Copy Markdown
Contributor
        Assert.Equal("TestValue1", config["TestKey1"]);

We could use the same key registered in refresh for further validation, instead of using two separate keys.


Refers to: tests/Tests.AzureAppConfiguration/RefreshTests.cs:521 in 11b77fd. [](commit_id = 11b77fd, deletion_comment = False)

Comment thread tests/Tests.AzureAppConfiguration/RefreshTests.cs Outdated
Comment thread tests/Tests.AzureAppConfiguration/RefreshTests.cs Outdated
Comment thread tests/Tests.AzureAppConfiguration/RefreshTests.cs
@avanigupta
Avani Gupta (avanigupta) deleted the avanigupta/updaterefreshtests branch October 16, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants