Skip to content

Extract common Unified Settings code and tests #6446

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

donnie-msft
Copy link
Contributor

@donnie-msft donnie-msft commented May 8, 2025

Bug

Fixes: NuGet/Home#14293

Description

I'm extracting a base class for the Unified Settings pages to implement. This will make each page smaller and cleaner, while maintaining a single source for the most common boilerplate code for the Unified Settings API. Tests also get a new base class which allows each page to be tested without copy/pasting tests from the base class.

  • Reduction of 47 lines of src/ code (just from the General/Configuration Files pages)
  • Removes ExternalSettingsUtility as all logic is now part of the base class.

Future Considerations

  • The tests are mostly covering exceptions now.
  • I'm deciding how I want to handle SetValueAsync tests. Could have a base test that invokes it for known Monikers or could have each test class test its own calls directly.
    • One problem for example, the General page sets values on PackageRestoreConsent, so the Mock Settings will have to fully implement what's necessary for that type.
      • There have never been tests that I noticed for those General setting test types, and that can be improved.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@donnie-msft donnie-msft requested a review from a team as a code owner May 8, 2025 16:18
jeffkl
jeffkl previously approved these changes May 8, 2025
jebriede
jebriede previously approved these changes May 8, 2025
Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

Love the abstract base class here! Few minor suggestions.

@donnie-msft donnie-msft dismissed stale reviews from jebriede and jeffkl via cf26a59 May 9, 2025 18:04
@donnie-msft donnie-msft force-pushed the dev-donnie-msft-usxExternalSettingsProvider branch from deb76e1 to cf26a59 Compare May 9, 2025 18:04
@donnie-msft
Copy link
Contributor Author

Thanks for the approvals. CI has been blocked for a few days. Just rebased and will merge once green.

@donnie-msft donnie-msft enabled auto-merge (squash) May 9, 2025 18:58
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.

Extract common Unified Settings code and tests
4 participants