Skip to content

Improve a test in Common.Certificates #1523

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Improve a test in Common.Certificates #1523

wants to merge 4 commits into from

Conversation

TimHess
Copy link
Member

@TimHess TimHess commented Jun 5, 2025

Description

Wrap RegisterChangeCallback in usings, reset changeCalled boolean between changes

Fixes #1506

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

@TimHess TimHess added Component/Common ReleaseLine/4.x Identified as a feature/fix for the 4.x release line labels Jun 5, 2025
@TimHess TimHess added this to the 4.0.0-rc1 milestone Jun 5, 2025
@bart-vmware
Copy link
Member

This is still wrong. Try debugging through it:

image

The changeCalled variable is set to true before the files get written.

@TimHess TimHess force-pushed the certificate_test branch from b6f0807 to a8fa12e Compare June 6, 2025 17:50
- more cert tests using both default and named options
- use SpinUntil instead of Task.Delay for file system operations
@TimHess TimHess force-pushed the certificate_test branch from a8fa12e to 2b58d74 Compare June 6, 2025 20:54
Copy link
Member

@bart-vmware bart-vmware left a comment

Choose a reason for hiding this comment

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

Thanks!

@bart-vmware
Copy link
Member

Should probably dispose X509Certificate2 instances in tests.

Copy link
Member

@bart-vmware bart-vmware left a comment

Choose a reason for hiding this comment

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

Reverting PR approval due to failing tests in cibuild.

@bart-vmware
Copy link
Member

bart-vmware commented Jun 11, 2025

Wouldn't it be easier to test via OptionsMonitor, such as in the test ChangeConfiguration_AppliesChanges, combined with MemoryFileProvider, such as in the test Reloads_options_on_change? Then the sleeps can be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component/Common ReleaseLine/4.x Identified as a feature/fix for the 4.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken test for Common.Certificates
2 participants