test(parity): consolidate the browse-commit guard onto one list; key browsers off DriverTypeNames (#558)
Two guards enumerated the same RawBrowseCommitMapper.BuildTagConfig seam against
two hand-authored lists, which can only drift. The older one's list was also
misnamed: DriverDispatchMapParityTests.NonBrowsableDriverTypes really meant
"types that produce the generic key" — AbLegacy is genuinely non-browsable yet
correctly absent from it, because it has a typed branch.
Retire the older fact. RawBrowseCommitMapperParityTests.GenericKeyExemptions is
now the single hand-authored list for this seam: named for what it holds, one
written reason per entry, and checked against a DI-derived browsable set rather
than a second list. The one assertion the retired fact made that nothing here
covered — set EQUALITY, i.e. an exemption must still DESCRIBE the mapper — moves
across as Every_generic_key_exemption_still_commits_to_the_generic_key. Without
it, a driver that gained a typed branch would keep a silently inert exemption.
Item 2: OpcUaClientDriverBrowser and GalaxyDriverBrowser returned string
literals ("OpcUaClient", "GalaxyMxGateway") where Sql/Mqtt use DriverTypeNames.
The values matched, and nothing guarded that. Repoint both at the constants —
DriverTypeNames.Galaxy carries the identical value — and add
Every_registered_browser_is_keyed_by_a_declared_driver_type_name over the same
AddAdminUI() read-back. Compared ORDINALLY: BrowserSessionService indexes on the
exact persisted string, and case-only drift (TwinCat/TwinCAT) is what
DriverTypeNames exists to end. Browser projects are out of reach of
DriverTypeNamesGuardTests, which pins the driver FACTORIES' names.
Falsified, four ways, each red naming the offender and green on revert:
- #555 re-run: disabling the Sql branch reddens the two enumeration facts
("...no dedicated BuildTagConfig branch...: Sql"), the same catch the
retired fact made.
- giving Modbus a typed branch reddens ONLY the new stale-exemption fact,
proving it is not redundant with the four that were already there.
- SqlDriverBrowser.DriverType => "SQL" and Galaxy => "GalaxyMXGateway" each
redden the new browser-key guard by name — case-only typos, which an
OrdinalIgnoreCase compare would have waved through.
This commit is contained in:
Joseph Doherty
@@ -4,6 +4,7 @@ using Microsoft.Extensions.Logging;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using ZB.MOM.WW.MxGateway.Client;
|
||||
using ZB.MOM.WW.OtOpcUa.Commons.Browsing;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config;
|
||||
using ZB.MOM.WW.Secrets.Abstractions;
|
||||
|
||||
@@ -47,9 +48,15 @@ public sealed class GalaxyDriverBrowser : IDriverBrowser
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
// Hardcoded literal: this project references Driver.Galaxy.Contracts, not Driver.Galaxy,
|
||||
// so GalaxyDriverFactoryExtensions.DriverTypeName isn't available here.
|
||||
public string DriverType => "GalaxyMxGateway";
|
||||
/// <remarks>
|
||||
/// Sourced from the shared <see cref="DriverTypeNames"/> set, reached through
|
||||
/// <c>Commons</c>. This project references Driver.Galaxy.Contracts, not Driver.Galaxy, so
|
||||
/// <c>GalaxyDriverFactoryExtensions.DriverTypeName</c> is not available here — but that was never a
|
||||
/// reason to hand-write the string, which is what this line used to do (Gitea #558):
|
||||
/// <see cref="DriverTypeNames.Galaxy"/> holds the same value and is the constant
|
||||
/// <c>DriverTypeNamesGuardTests</c> already pins to the factory's registration.
|
||||
/// </remarks>
|
||||
public string DriverType => DriverTypeNames.Galaxy;
|
||||
|
||||
// Deserializes a GalaxyDriverOptions blob, opens a transient GalaxyRepositoryClient
|
||||
// against the configured gateway endpoint, and returns a browse session over it. The
|
||||
|
||||
+8
-1
@@ -6,6 +6,7 @@ using Opc.Ua;
|
||||
using Opc.Ua.Client;
|
||||
using Opc.Ua.Configuration;
|
||||
using ZB.MOM.WW.OtOpcUa.Commons.Browsing;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; // OpcUaClientDriverOptions + NamespaceMap
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser;
|
||||
@@ -41,7 +42,13 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public string DriverType => "OpcUaClient";
|
||||
/// <remarks>
|
||||
/// Sourced from the shared <see cref="DriverTypeNames"/> set rather than the hand-written literal
|
||||
/// this line used to carry (Gitea #558). The value is unchanged; the constant is what
|
||||
/// <c>DriverTypeNamesGuardTests</c> pins to the factory's own registration, so a rename on either
|
||||
/// side can no longer leave this browser indexed under a key nothing dispatches to.
|
||||
/// </remarks>
|
||||
public string DriverType => DriverTypeNames.OpcUaClient;
|
||||
|
||||
/// <inheritdoc />
|
||||
public async Task<IBrowseSession> OpenAsync(string configJson, CancellationToken cancellationToken)
|
||||
|
||||
@@ -271,7 +271,9 @@ public static class RawBrowseCommitMapper
|
||||
// and holds this line to two claims rather than to the assertion the comment used to make:
|
||||
// - a driver type with a bespoke IDriverBrowser registered by AddAdminUI() — read back from the
|
||||
// real registration, not a list — must NOT reach here; and
|
||||
// - any type that does reach here must be named in that test's GenericKeyExemptions with a reason.
|
||||
// - any type that does reach here must be named in that test's GenericKeyExemptions with a reason,
|
||||
// and every name on that list must still reach here (Gitea #558) — an exemption for a driver
|
||||
// that has since gained a branch above is a stale claim, and a silently inert one.
|
||||
// The universal DiscoveryDriverBrowser's arm is not derivable from the AdminUI assembly (it needs
|
||||
// the runtime drivers + the Host's IDriverFactory), so it is covered by the exemption list rather
|
||||
// than mechanically — see the test's remarks for the residual trust that leaves (Gitea #555).
|
||||
|
||||
@@ -1,16 +1,28 @@
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
|
||||
using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
|
||||
|
||||
/// <summary>
|
||||
/// Parity guards for the two remaining per-driver dispatch maps (<c>deferment.md</c> G-5, G-6).
|
||||
/// Both were already testable structures — unlike the two Razor <c>@switch</c> blocks that needed
|
||||
/// extracting first — so nothing but the tests was missing.
|
||||
/// Parity guards for the per-driver <b>CSV column</b> dispatch map (<c>deferment.md</c> G-5). It was
|
||||
/// already a testable structure — unlike the two Razor <c>@switch</c> blocks that needed extracting
|
||||
/// first — so nothing but the tests was missing.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <b>The browse-commit seam (G-6) is guarded next door, not here (Gitea #558).</b> This class used to
|
||||
/// also hold <c>Browse_commit_falls_back_to_the_generic_address_key_for_exactly_the_non_browsable_drivers</c>,
|
||||
/// over the same <c>RawBrowseCommitMapper.BuildTagConfig</c> seam
|
||||
/// <see cref="RawBrowseCommitMapperParityTests"/> now enumerates. Two guards over one seam means two
|
||||
/// hand-authored lists of "drivers that legitimately reach the generic key", and the pair can only
|
||||
/// drift — this one's was additionally MISNAMED (<c>NonBrowsableDriverTypes</c> really meant "types
|
||||
/// that produce the generic key": <c>AbLegacy</c> is genuinely non-browsable yet correctly absent,
|
||||
/// because it has a typed branch). The surviving list is
|
||||
/// <c>RawBrowseCommitMapperParityTests.GenericKeyExemptions</c>, which is named for what it holds,
|
||||
/// carries a written reason per entry, and is checked against a <b>derived</b> browsable set rather
|
||||
/// than a second hand-authored one.
|
||||
/// </remarks>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class DriverDispatchMapParityTests
|
||||
{
|
||||
@@ -47,52 +59,4 @@ public sealed class DriverDispatchMapParityTests
|
||||
|
||||
stale.ShouldBeEmpty($"CsvColumnMap has entries for undeclared driver types: {string.Join(", ", stale)}");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Every driver type either produces a TYPED blob from browse-commit, or is on the documented
|
||||
/// non-browsable list. The generic <c>{"address": …}</c> fallback opens EMPTY in a typed editor and
|
||||
/// blanks the field on save — the MTConnect branch was added for exactly that, and Sql had the same
|
||||
/// bug unnoticed (<c>deferment.md</c> G-6) because the fallback's comment claimed "browsable drivers
|
||||
/// are all handled above", which stopped being true the moment <c>SqlDriverBrowser</c> registered.
|
||||
/// <para><b>Asserted in both directions on purpose.</b> A one-way check would let a newly-browsable
|
||||
/// driver be quietly added to the exclusion list instead of getting a branch. Requiring the two sets
|
||||
/// to match exactly means a driver gaining or losing a browser forces this list to be revisited.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Browse_commit_falls_back_to_the_generic_address_key_for_exactly_the_non_browsable_drivers()
|
||||
{
|
||||
// Address fields as a browser supplies them; a driver whose browser states none must still produce
|
||||
// something its typed editor can read from the leaf name alone.
|
||||
var addressFields = new Dictionary<string, string>(StringComparer.Ordinal)
|
||||
{
|
||||
["schema"] = "dbo",
|
||||
["table"] = "Readings",
|
||||
["columnName"] = "Speed",
|
||||
};
|
||||
|
||||
var fellBack = DriverTypeNames.All
|
||||
.Where(t => RawBrowseCommitMapper.BuildTagConfig(t, "Speed", addressFields) == """{"address":"Speed"}""")
|
||||
.OrderBy(t => t, StringComparer.Ordinal)
|
||||
.ToList();
|
||||
|
||||
fellBack.ShouldBe(
|
||||
NonBrowsableDriverTypes.OrderBy(t => t, StringComparer.Ordinal).ToList(),
|
||||
$"the set of driver types committing to the generic \"address\" key drifted. Extra entries are "
|
||||
+ $"browsable drivers whose typed editor will open EMPTY and blank the field on save; missing "
|
||||
+ $"entries gained a typed branch and should leave this list. Got: {string.Join(", ", fellBack)}");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Driver types with no browser, for which the generic <c>address</c> key is the correct commit.
|
||||
/// <list type="bullet">
|
||||
/// <item><b>Modbus</b> — register addresses are numeric coordinates, not a browsable namespace.</item>
|
||||
/// <item><b>Calculation</b> — a pseudo-driver over other tags' RawPaths; there is no device.</item>
|
||||
/// </list>
|
||||
/// Adding a type here is a claim that it cannot be browsed. If it can, give it a branch instead.
|
||||
/// </summary>
|
||||
private static readonly string[] NonBrowsableDriverTypes =
|
||||
[
|
||||
DriverTypeNames.Modbus,
|
||||
DriverTypeNames.Calculation,
|
||||
];
|
||||
}
|
||||
|
||||
@@ -30,6 +30,15 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
|
||||
/// class walks <see cref="DriverTypeNames.All"/>, so a newly-registered driver is triaged the first time
|
||||
/// the suite runs — the idiom <c>DriverFormMapParityTests</c> / <c>DriverDispatchMapParityTests</c> use.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>The only guard over this seam (Gitea #558).</b> <c>DriverDispatchMapParityTests</c> briefly held a
|
||||
/// second one, enumerating the same mapper against its own hand-authored
|
||||
/// <c>NonBrowsableDriverTypes</c>. Two lists for one question can only drift, so it was retired into
|
||||
/// this class: <see cref="GenericKeyExemptions"/> is now the single hand-authored list for browse-commit,
|
||||
/// and the one assertion the old fact made that nothing here covered — that an exemption still describes
|
||||
/// the mapper — came with it as
|
||||
/// <see cref="Every_generic_key_exemption_still_commits_to_the_generic_key"/>.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
@@ -138,6 +147,70 @@ public sealed class RawBrowseCommitMapperParityTests
|
||||
+ "Remove the exemption and give the driver a BuildTagConfig branch.");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The other direction: an exemption must still DESCRIBE the mapper. A driver that has since gained
|
||||
/// a typed <c>BuildTagConfig</c> branch no longer reaches the generic key, so its exemption is a
|
||||
/// stale claim — and worse, a silently inert one, since the enumeration above only ever flags
|
||||
/// drivers that are <em>missing</em> from the list.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Inherited from <c>DriverDispatchMapParityTests</c>'s retired browse-commit fact, which asserted
|
||||
/// set EQUALITY against its own hand-authored list and so covered this direction as a side effect
|
||||
/// (Gitea #558). Consolidating onto one list would have dropped the assertion had it not moved here.
|
||||
/// Keeping it also keeps the list honest as a piece of documentation: every reason written in
|
||||
/// <see cref="GenericKeyExemptions"/> is a live description of what the mapper does today, not an
|
||||
/// archived one.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void Every_generic_key_exemption_still_commits_to_the_generic_key()
|
||||
{
|
||||
var superseded = GenericKeyExemptions
|
||||
.Where(e => !CommitsToTheGenericAddressKey(e.DriverType))
|
||||
.OrderBy(e => e.DriverType, StringComparer.Ordinal)
|
||||
.ToList();
|
||||
|
||||
superseded.ShouldBeEmpty(
|
||||
"these driver types are exempted from needing a typed branch, but RawBrowseCommitMapper now "
|
||||
+ $"gives them one: {string.Join(", ", superseded.Select(e => e.DriverType))}. The exemption is "
|
||||
+ "stale — delete it, so the list keeps meaning what it says.");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Every registered browser is indexed under a <see cref="DriverTypeNames"/> value, exactly.
|
||||
/// <c>BrowserSessionService</c> looks a browser up by the driver type persisted on the device, so a
|
||||
/// browser whose key is anything else is registered but unreachable — the picker reports "no browser
|
||||
/// for this driver" and the operator falls back to typing the address by hand.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Added with Gitea #558, alongside repointing <c>OpcUaClientDriverBrowser</c> and
|
||||
/// <c>GalaxyDriverBrowser</c> off string literals (<c>"OpcUaClient"</c> / <c>"GalaxyMxGateway"</c>)
|
||||
/// onto the constants. The values matched, so nothing was broken — but nothing prevented the next
|
||||
/// browser from hand-writing its key either, and a browser project cannot be reached by
|
||||
/// <c>DriverTypeNamesGuardTests</c>, which pins the driver FACTORIES' names rather than the
|
||||
/// browsers'. This is the seam where the two meet: the same DI read-back the guards above use
|
||||
/// already has every browser instance in hand.
|
||||
/// <para>
|
||||
/// Compared ORDINALLY, not case-insensitively: <c>BrowserSessionService</c>'s index and the
|
||||
/// persisted <c>DriverType</c> must agree on the exact string, and "differs only in case" is
|
||||
/// precisely the <c>TwinCat</c>/<c>TwinCAT</c> drift <see cref="DriverTypeNames"/> exists to end.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void Every_registered_browser_is_keyed_by_a_declared_driver_type_name()
|
||||
{
|
||||
var declared = DriverTypeNames.All.ToHashSet(StringComparer.Ordinal);
|
||||
|
||||
var unreachable = RegisteredBespokeBrowserDriverTypes()
|
||||
.Where(t => !declared.Contains(t))
|
||||
.OrderBy(t => t, StringComparer.Ordinal)
|
||||
.ToList();
|
||||
|
||||
unreachable.ShouldBeEmpty(
|
||||
"these IDriverBrowser registrations are keyed by a string that is not a DriverTypeNames member, "
|
||||
+ $"so BrowserSessionService can never match a device to them: {string.Join(", ", unreachable)}. "
|
||||
+ "Use the DriverTypeNames constant.");
|
||||
}
|
||||
|
||||
/// <summary>An exemption for a driver type that no longer exists is stale bookkeeping.</summary>
|
||||
[Fact]
|
||||
public void Every_generic_key_exemption_names_a_declared_driver_type()
|
||||
|
||||
Reference in New Issue
Block a user