-
Notifications
You must be signed in to change notification settings - Fork 1k
Make GDS tests stable and add better logging to unit tests #3271
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Stabilizes GDS/server-related tests and enhances observability by routing previous Console output through structured logging, introducing diagnostics mask configuration, and refactoring telemetry/session creation. Key changes include: (1) pervasive replacement of direct certificate factory usage and constructor overload removals, (2) added diagnostics/telemetry plumbing (ReturnDiagnostics masks, activity source changes), and (3) retry / connection loop refactors plus logging improvements.
Reviewed Changes
Copilot reviewed 75 out of 75 changed files in this pull request and generated 12 comments.
Show a summary per file
File | Description |
---|---|
Tests/Opc.Ua.Server.Tests/* | Removed explicit telemetry injections; rely on default NUnitTelemetryContext and updated activity source usage. |
Tests/Opc.Ua.Gds.Tests/* | Logging replaces Console writes; GDS test startup simplified; constructors adjusted for new telemetry patterns. |
Tests/Opc.Ua.Client.Tests/* | Added diagnostics masks to session creation; improved logging; modified retry logic in fixtures. |
Tests/Opc.Ua.Client.ComplexTypes.Tests/* | MockResolver no longer needs telemetry; updated dictionary loading with cancellation token. |
Tests/Common/Logging.cs | Reworked NUnit and BenchmarkDotNet logger providers; simplified creation API and added exception formatting helpers. |
Stack/Opc.Ua.Core/* | Substantial refactors: certificate parsing now uses X509CertificateLoader, ServiceResult / Exception formatting changes, removal of ActivitySource static, added diagnostics handling, updated cryptography & error helpers. |
Libraries/Opc.Ua.* | Added logging, diagnostics mask plumbing, retry logic refactors, disposal guards, certificate & identity handling changes, and improved error logging. |
Applications/Quickstarts.* | Updated to new UserIdentity constructors (removed telemetry parameter) and logging changes. |
Directory.Packages.props | Minor package version bumps. |
.editorconfig | Expanded analyzer configuration and documentation / formatting comments. |
Comments suppressed due to low confidence (8)
Libraries/Opc.Ua.Gds.Server.Common/CertificateGroup.cs:525
- The CA certificate is wrapped in a using statement then stored (via intermediate assignments not shown) and later returned from Certificates[certificateType]; disposing it here leaves a disposed instance in the cache. Remove the using and manage disposal only for truly temporary instances (or clone before disposing).
using X509Certificate2 certificate = TryGetECCCurve(certificateType, out ECCurve curve)
? builder.SetECCurve(curve).CreateForECDsa()
: builder
.SetHashAlgorithm(
X509Utils.GetRSAHashAlgorithmName(Configuration.CACertificateHashSize))
.SetRSAKeySize(Configuration.CACertificateKeySize)
.CreateForRSA();
#else
using X509Certificate2 certificate = builder
.SetHashAlgorithm(
X509Utils.GetRSAHashAlgorithmName(Configuration.CACertificateHashSize))
.SetRSAKeySize(Configuration.CACertificateKeySize)
.CreateForRSA();
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs
Outdated
Show resolved
Hide resolved
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
…f x509Certificates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beyond the comments I posted, there are a lot of places where lock() statements are used that are not guaranteed to be called from synchronous methods only. These also need to be reviewed and preferrably made async-aware.
Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Gds.Client.Common/ServerPushConfigurationClient.cs
Outdated
Show resolved
Hide resolved
…ad of just export.
Proposed changes
The following changes are contained in this pull request:
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...