-
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
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3271 +/- ##
==========================================
+ Coverage 57.81% 57.85% +0.03%
==========================================
Files 365 365
Lines 79423 79723 +300
Branches 13870 13852 -18
==========================================
+ Hits 45920 46123 +203
- Misses 29338 29459 +121
+ Partials 4165 4141 -24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…f x509Certificates
salihgoncu
left a comment
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.
|
@marcschier Sorry to bother again, this PR starts to touch many areas, can you split those two into separate stable ones, we can merge immediately imo:
I think those are significant enough, so they should be in the history as own commits. |
|
@romanett - I will not have time to split this once more - this was already split from the switch statements (which had nothing to do with the test stability). This PR addresses the flakiness we see (GDS, certificates) and allows debugging of the issues in the pipelines (logging) and getting it split up again takes me too much time because the tests on the partial change would still be so flaky that the tests need to be restarted numerous times. |
salihgoncu
left a comment
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.
This iteration looks much better. - I' fine with it, if nobody has any objections.
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
| { | ||
| if (session == Session) | ||
| { | ||
| Session.Dispose(); |
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.
I think we should attempt a disconnect here.
Actually it would probably be better to extract the Dispose(&Disconnect) out from the lock/release block, like we do in DisconnectAsync() above.
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.
At this point the session is likely already defunct, and since this is a free threaded callback the less time we spend here the better, let's leave it like this for now IMO.
| { | ||
| if (Session != null) | ||
| { | ||
| Session.Dispose(); |
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.
let's gracefully disconnect the session before disposing
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.
I think it is fine to just close the channel as the session is a) defunct, and being reconnected, b) customer has already called DisconnectAsync. Anything in between can be handled with some non-graceful behavior IMO. The code would become a bit unwieldy if we allow disconnect during connect, but in general the usability of these client classes leaves something to be deserved (properties, connect, disconnect, etc.).
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
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin 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...