Skip to content

Improve Common.Certificates internals and tests #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.

Sign up for GitHub

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

Jump to bottom
Merged
TimHess merged 15 commits into from
Jul 1, 2025
Merged

Improve Common.Certificates internals and tests #1523

TimHess merged 15 commits into from
Jul 1, 2025

Conversation

@TimHess
Copy link
Member

@TimHess TimHess commented Jun 5, 2025 •
edited
Loading

Description

Handle exceptions when loading certificates in IOptions, fix named options bug, simplify related code.
Enhance test scenario coverage and implementations, ensure testing for both default and named options.

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

bart-vmware commented Jun 6, 2025

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 2 times, most recently from a8fa12e to 2b58d74 Compare June 6, 2025 20:54
bart-vmware
bart-vmware approved these changes Jun 10, 2025
View reviewed changes
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

bart-vmware commented Jun 10, 2025

Should probably dispose X509Certificate2 instances in tests.

bart-vmware
bart-vmware requested changes Jun 10, 2025
View reviewed changes
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 •
edited
Loading

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.

@TimHess TimHess force-pushed the certificate_test branch from e362560 to 4135edb Compare June 25, 2025 20:09
@TimHess
Copy link
Member Author

TimHess commented Jun 25, 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.

I'm not sure it's worth the effort, or exactly how we would use TestOptionsMonitor in this situation when we are also depending on IConfigureNamedOptions to read the certificate from disk, and there's code in there to exit early if the certificate is not null (which it is unless we do something to null it out).

I do like the idea of using MemoryFileProvider, but the problem is that FilePathInOptionsChangeTokenSource depends on the private class FileSystemChangeWatcher which only works with PhysicalFilesWatcher. I see an option on PhysicalFilesWatcher to pass in a FileSystemWatcher, but that still seems to be tied to a physical file system (all the tests are using TempDirectory rather than any form of in-memory option), so I'm not sure how to change this without adding some additional abstraction to wrap PhysicalFilesWatcher

@bart-vmware
Copy link
Member

bart-vmware commented Jun 26, 2025 •
edited
Loading

I do like the idea of using MemoryFileProvider, but the problem is that FilePathInOptionsChangeTokenSource depends on the private class FileSystemChangeWatcher which only works with PhysicalFilesWatcher. I see an option on PhysicalFilesWatcher to pass in a FileSystemWatcher, but that still seems to be tied to a physical file system (all the tests are using TempDirectory rather than any form of in-memory option), so I'm not sure how to change this without adding some additional abstraction to wrap PhysicalFilesWatcher

Good point, I didn't realize that limitation. It's unfortunate that we can't use MemoryFileProvider due to that, but I don't think it can be easily solved, so I'm okay with the original approach.

bart-vmware
bart-vmware requested changes Jun 26, 2025
View reviewed changes
@TimHess TimHess changed the title Improve a test in Common.Certificates Improve Common.Certificates internals and tests Jun 26, 2025
TimHess added 2 commits June 26, 2025 15:20
@TimHess
PR feedback and ideas
7d55822 
- inline AddCertificate(), use appsetting.json in tests for path config changes
- catch & log exceptions when loading certificates
- poll for changes during tests
- check all the changetokens
- more naming changes
@TimHess
"using" for X509Certificate2
563c90d
@TimHess TimHess marked this pull request as ready for review June 26, 2025 20:34
@TimHess TimHess requested a review from bart-vmware June 26, 2025 20:34
@TimHess TimHess self-assigned this Jun 26, 2025
bart-vmware
bart-vmware reviewed Jun 27, 2025
View reviewed changes
@TimHess TimHess force-pushed the certificate_test branch from cd62b02 to cd6f434 Compare June 27, 2025 16:46
@TimHess TimHess requested a review from bart-vmware June 27, 2025 17:26
@TimHess
Add delay in filewatcher token
b41cea5 
- take error handling back out of ConfigureCertificateOptions
- reduce polling timeout in tests
@TimHess TimHess force-pushed the certificate_test branch from a8142b9 to b41cea5 Compare June 30, 2025 18:44
bart-vmware
bart-vmware requested changes Jul 1, 2025
View reviewed changes
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.

LGTM, aside from a redundant assertion. Please rebase on main.

@TimHess TimHess requested a review from bart-vmware July 1, 2025 12:52
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 1, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@TimHess TimHess merged commit a658936 into main Jul 1, 2025
22 checks passed
@TimHess TimHess deleted the certificate_test branch July 1, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bart-vmware bart-vmware bart-vmware approved these changes

Assignees

@TimHess TimHess

Labels

Component/Common ReleaseLine/4.x Identified as a feature/fix for the 4.x release line

Projects

None yet

Milestone

4.0.0-rc1

Development

Successfully merging this pull request may close these issues.

Broken test for Common.Certificates

3 participants

@TimHess @bart-vmware