Skip to content

Address WSS discovery review feedback and follow-up tests - #4178

Merged
marcschier merged 9 commits into
masterfrom
copilot/fix-comments-in-review-thread
Aug 4, 2026
Merged

Address WSS discovery review feedback and follow-up tests#4178
marcschier merged 9 commits into
masterfrom
copilot/fix-comments-in-review-thread

Conversation

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

Addresses the review feedback for application-aware WSS discovery and incorporates the follow-up coverage from #4179.

  • Observe the returned task when asserting null ApplicationConfiguration validation with Assert.ThrowsAsync.
  • Correct the discovery URL comment grammar.
  • Verify WSS discovery uses the application's certificate validator by rejecting an untrusted discovery certificate and asserting the TLS authentication failure.
  • Connect the anonymous WSS binary test through the discovered ConfiguredEndpoint, then verify the selected transport profile and security mode.

Related Issues

Checklist

  • I have signed the CLA and read the CONTRIBUTING doc.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added all necessary documentation.
  • I have verified that my changes do not introduce (new) build or analyzer warnings.
  • I ran all tests locally using the UA.slnx solution against at least .net framework and .net 10, and all passed.
  • I fixed all failing and flaky tests in the CI pipelines and all CodeQL warnings.
  • I have addressed all PR feedback received.

@CLAassistant

CLAassistant commented Aug 4, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ o6-jpfr
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI and others added 3 commits August 4, 2026 08:51
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
ConfiguredEndpoint refreshed endpoint descriptions with a DiscoveryClient
created only from EndpointConfiguration. That path did not receive the
application certificate manager, so WSS discovery could not validate a
server certificate trusted by the application before opening a session.

Add an ApplicationConfiguration-aware endpoint refresh overload and use it
from the default and channel-manager session paths. Preserve the existing
overloads for compatibility.

The application-aware DiscoveryClient now supplies the configured
certificate validation for secure discovery transports while TCP and legacy
callers retain their existing behavior.

Specification:
https://reference.opcfoundation.org/Core/Part6/v105/docs/7.5.3

Tests: UpdateFromServerWithNullApplicationConfigurationThrowsArgumentNullException
(1 passed, net10.0)

Tests: UpdateBeforeConnectUsesApplicationCertificateValidationAsync
(1 passed, net10.0)

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix code based on review comments Address WSS discovery review feedback Aug 4, 2026
Copilot AI requested a review from marcschier August 4, 2026 08:54
@marcschier

Copy link
Copy Markdown
Collaborator

@copilot fix conflicts with master, then merge #4179 into this and update PR title and description. Close #4179 when done

Copilot AI and others added 3 commits August 4, 2026 08:57
# Conflicts:
#	src/Opc.Ua.Core/Stack/Configuration/ConfiguredEndpoints.cs
#	tests/Opc.Ua.Core.Tests/Stack/Client/ConfiguredEndpointTests.cs

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
# Conflicts:
#	src/Opc.Ua.Core/Stack/Configuration/ConfiguredEndpoints.cs
#	tests/Opc.Ua.Core.Tests/Stack/Client/ConfiguredEndpointTests.cs

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
…tests

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
@marcschier
marcschier marked this pull request as ready for review August 4, 2026 09:29
Copilot AI review requested due to automatic review settings August 4, 2026 09:29
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses prior review feedback around application-aware WSS discovery by fixing an async exception assertion in tests and cleaning up a small grammar issue in a discovery-URL comment.

Changes:

  • Update the ConfiguredEndpointTests null-configuration assertion to use Assert.ThrowsAsync so the returned task is properly observed.
  • Fix grammar in an inline comment in ConfiguredEndpoints.UpdateFromServerCoreAsync related to discovery URL handling.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/Opc.Ua.Core.Tests/Stack/Client/ConfiguredEndpointTests.cs Switches the null ApplicationConfiguration validation test to Assert.ThrowsAsync for correct async exception testing.
src/Opc.Ua.Core/Stack/Configuration/ConfiguredEndpoints.cs Minor grammar correction in a discovery URL comment within the server update path.

Comment thread tests/Opc.Ua.Core.Tests/Stack/Client/ConfiguredEndpointTests.cs
Copilot AI requested a review from marcschier August 4, 2026 09:51
@marcschier marcschier changed the title Address WSS discovery review feedback Address WSS discovery review feedback and follow-up tests Aug 4, 2026
@marcschier marcschier mentioned this pull request Aug 4, 2026
7 tasks

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix conflicts with master, then merge #4179 into this and update PR title and description. Close #4179 when done

Merged current master and the latest #4179 head into #4178 as true merge commits, then updated #4178's title and description. The affected net10.0 test suites pass (4,145 Core and 777 Sessions tests), and both affected projects build cleanly for net48. I also marked #4179 as superseded; the GitHub write API is currently refusing to close that draft PR because it cannot resolve the PR node, so #4179 still needs to be closed manually.

@marcschier
marcschier merged commit 5a690c5 into master Aug 4, 2026
124 of 201 checks passed
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.

6 participants