Skip to content
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

Add a unit test to cover the unified setting's registration file. #72946

Merged
merged 28 commits into from
Apr 12, 2024

Conversation

Cosifne
Copy link
Member

@Cosifne Cosifne commented Apr 9, 2024

This is a test-only change.
Originally mentioned by Sam somewhere we need some tests to guard the registration files of Unified Settings.
This PR adds unit test to cover the C# Intellisense page.
It covers

  1. Type
  2. Default value
  3. The storage path to ISettingManager. Make sure the value is correct and matches
    public static readonly IReadOnlyDictionary<string, VisualStudioOptionStorage> Storages = new Dictionary<string, VisualStudioOptionStorage>()
  4. The enum to int mapping if the option type is Enum

This would ensure we don't make simple mistakes like typo in the registration file.

The general test code is put under Microsoft.VisualStudio.LanguageServices.UnitTests, and the real test is put under Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests, so that later we can onboard the VB tests easily.

@Cosifne Cosifne requested a review from a team as a code owner April 9, 2024 00:14
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 9, 2024
@Cosifne
Copy link
Member Author

Cosifne commented Apr 9, 2024

Kindly tag @sharwell to review.


' Mapping from the config name to its path in Unified Settings registration file
Private Shared ReadOnly s_unifiedSettingsStorage As New Dictionary(Of String, UnifiedSettingsStorage)() From {
{"dotnet_trigger_completion_on_typing_letters", New UnifiedSettingsStorage("textEditor.%LANGUAGE%.intellisense.triggerCompletionOnTypingLetters")},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Is this mapping not anywhere else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not now at least.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They might be moved to

public static readonly IReadOnlyDictionary<string, VisualStudioOptionStorage> Storages = new Dictionary<string, VisualStudioOptionStorage>()
in future
Unified Settings provides Microsoft.VisualStudio.Utilities.UnifiedSettings.ISettingsManager, which would replace the usage of all old ISettingsManager2 IVsFeatureFlag and etc... but it's not required now.

@Cosifne Cosifne requested a review from sharwell April 9, 2024 19:52
@Cosifne Cosifne enabled auto-merge April 11, 2024 18:48
@Cosifne
Copy link
Member Author

Cosifne commented Apr 12, 2024

/azp run roslyn-integratoin-CI

Copy link

No pipelines are associated with this pull request.

@Cosifne Cosifne merged commit adc0ca9 into dotnet:main Apr 12, 2024
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 12, 2024
@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants