FreeAndNil commented on PR #326:
URL: https://github.com/apache/logging-log4net/pull/326#issuecomment-6122088334
Implementation plan for #326, kept here for the record. Revised after the
merge so that it describes the change as it landed.
<details>
<summary>Plan</summary>
# PR #326: one home for the reflection in the tests
## Context
`log4net` grants no `InternalsVisibleTo`, so `log4net.Tests` reaches private
and internal members by
reflection. That stays. What has grown around it is 27 hand written lookups
over 13 files, each
repeating the same three steps differently:
- the `BindingFlags` pair is written out every time, in both orders
- a missing member is reported four ways: bare `!`
(`NullReferenceException`),
`?? throw new InvalidOperationException("... is missing")`, `?? throw new
InvalidOperationException("... no longer exists - update this test along
with it.")`, and
`?? throw new MissingMethodException(...)`
- the cast and the `!` on `Invoke` repeat at every call site
Goal: one `ReflectionExtensions` in the test assembly, one readable line per
call site, one
diagnostic when a member is renamed. No production change, no new
dependency, no behaviour change.
Out of scope: `log4net.Ext.Mail.Tests`, which has `InternalsVisibleTo` and
uses no reflection.
## The new file
`src/log4net.Tests/Util/ReflectionExtensions.cs`, `internal static class`,
**`namespace
log4net.Tests;`** rather than `log4net.Tests.Util`. Name lookup walks
enclosing namespaces, so the
root namespace makes the extensions visible in `log4net.Tests.Appender`,
`.Core`, `.Config` and
`.Util` with no `using` line anywhere. One sentence of doc per member.
One name per operation, static and instance told apart by the receiver: the
`Type` member is the
static one, the `object` member the instance one. `Type` is more derived
than `object`, so
`typeof(X).Invoke<T>(...)` binds to the `Type` member with no cast and no
separate name.
**C# 14 extension blocks**, one block per receiver, so the split is visible
in the file layout
rather than repeated in every signature:
```csharp
internal static class ReflectionExtensions
{
extension(Type owner)
{
internal MethodInfo NonPublicMethod(string name, params Type[] signature)
internal FieldInfo NonPublicField(string name)
internal Type NonPublicNestedType(string name)
internal T Invoke<T>(string name, params object?[] arguments)
internal void Invoke(string name, params object?[] arguments)
internal T GetFieldValue<T>(string name)
internal void SetFieldValue(string name, object? value)
internal T? GetPropertyValue<T>(string name)
internal T Construct<T>(params object?[] arguments)
}
extension(object target)
{
internal T Invoke<T>(string name, params object?[] arguments)
internal void Invoke(string name, params object?[] arguments)
internal T GetFieldValue<T>(string name)
internal void SetFieldValue(string name, object? value)
}
extension(Assembly assembly)
{
internal Type NonPublicType(string fullName)
}
}
```
`Invoke` picks the overload itself: every method of that name with the right
parameter count, over
the type and its bases, and only when more than one remains, the one whose
parameters accept the
runtime arguments (a null fits any reference or nullable parameter). Still
ambiguous, it throws
naming the count and asks for a signature. So no call site spells out a
signature. `NonPublicMethod` keeps its explicit signature for
`AotCompatibilityTest`, which asserts on the `MethodInfo`.
The one trap: a variable statically typed `object` that happens to hold a
`Type` would bind to the
instance member. No call site does that; note it in the class doc comment.
Rules the implementation follows:
- **One diagnostic, the existing best one.** Every miss throws
`InvalidOperationException($"{owner.FullName}.{name} no longer exists -
update this test along
with it.")`, the message `SystemInfoTest` and `AotCompatibilityTest`
already use. That kills the
bare `!` sites, where a rename surfaces as `NullReferenceException` on a
plumbing line.
- **Flags are `Static | Instance | Public | NonPublic`**, so no call site
chooses them. `Public`
has to be in the set: `LockingStreamTest` invokes the *public*
`AcquireLock`/`ReleaseLock` on the
non-public nested `FileAppender.LockingStream`, and
`AotCompatibilityTest.Parameter` resolves a
public method. A private static and a private instance member cannot share
a name in one type,
so nothing becomes ambiguous. The `NonPublic` in the names says what the
sites are for, not what
the flags exclude; say so in the class doc comment.
- **An empty `signature` must not reach the five argument `GetMethod`.**
`Type[0]` there means
"the parameterless overload", not "by name alone". `NonPublicMethod`
branches:
`signature.Length == 0` uses `GetMethod(name, flags)`, otherwise
`GetMethod(name, flags, null, signature, null)`.
- **Arguments are passed as an explicit `[a, b]`, never as an expanded
`params` list.** A C# 14
extension block miscomputes the nullability of an expanded `params`
argument and reports CS8620
on every reference-typed one, which `WarningsAsErrors=nullable` turns into
a build error. The
identical classic `this Type owner` extension method compiles clean, so
this is the block syntax
alone. The `params` stays, so a no-argument call needs no brackets.
- **No list patterns.** `System.Index` does not exist on net462, so
`candidates switch { [] => ... }`
fails with CS0518 there. Plain `Count` checks instead.
- **The lookups walk `BaseType`.** A private member is not inherited, and
the instance overloads
start from `target.GetType()`, which can be a subclass.
- **`GetMethod(name, BindingFlags, Type[])` is net5.0 and later**, and
`log4net.Tests` targets
`net462;net10.0`, so the five argument overload is the only portable one
with a signature. That
is what `FileAppenderMutexNameTest` already does. `XmlConfiguratorTest`
uses the three argument
overload today and gets away with it only because the whole file sits
inside
`#if NET8_0_OR_GREATER`; the shared helper has no such cover.
- `Invoke<T>` casts the result, so a site reads `string escaped =
EscapePercent(message)` with the
cast and the `!` gone.
- `GetPropertyValue<T>` returns `T?`: `SystemInfoTest.IsAndroid` asserts on
a boxed `bool`
compared with `Is.False`, and today tolerates a null property via `?.`.
- A single `null` argument must be written `new object?[] { null }`, since
`params` reads a bare
`null` as the array. No current site passes a literal null, so this is a
doc line only.
## The call sites
13 files, mechanical, one commit. The shape, from `LocalSyslogAppenderTest`:
```csharp
// before
private static string EscapePercent(string message)
=> (string)typeof(LocalSyslogAppender)
.GetMethod("EscapePercent", BindingFlags.Static |
BindingFlags.NonPublic)!
.Invoke(null, [message])!;
// after
private static string EscapePercent(string message)
=> typeof(LocalSyslogAppender).Invoke<string>(nameof(EscapePercent),
[message]);
```
**Pass `nameof(Wrapper)` rather than a string literal wherever the wrapper
carries the member's
name**, which is most of the method sites: `nameof` on the enclosing method
group is legal and
works with overloads, so `MutexNameForPath` gets it twice.
`LoggingEventTest` and `TransformTest`
already do this. It does not reach the field sites: private fields are
`_camelCase`, so no wrapper
can share their name, and a literal stays there. Nor the three sites that
have no wrapper at all:
`LockingStreamTest`'s `AcquireLock`/`ReleaseLock`, `LevelMappingTest`'s
`SortEntries` and
`XmlConfiguratorTest`'s `InternalConfigure`.
Not `[CallerMemberName]`: it cannot follow `params`, it returns the
enclosing *method* name for a
local function such as `LoggingEventTest.ReviseThreadName`, and it hides the
coupling that the
wrapper exists to make visible.
The per file helpers (`EscapePercent`, `SplitLines`, `PrepareEventText`,
`MutexNameForPath`, ...)
**stay**: they give the test its readable line and they are where the
private member's name is
written down once. Only their bodies collapse.
| file | sites | what they become |
| --- | --- | --- |
| `Appender/LocalSyslogAppenderTest.cs` | 7 | `Invoke<string>(nameof(...),
[message])` / `Invoke<string[]>` for the four escape helpers,
`GetFieldValue<IntPtr>("_handleToIdentity")` for `CurrentIdentityHandle`,
`NonPublicField` where the `FieldInfo` is asserted on,
`Assembly.NonPublicType(...)` for `ContentEscape` and `NativeMethods`, the
latter with `NonPublicMethod("syslog")` |
| `Appender/LockingStreamTest.cs` | 5 | `appender.SetFieldValue("_stream",
stream)`, `appender.Invoke(nameof(RunWithBestEffortLock), [action])`,
`typeof(FileAppender).NonPublicNestedType("LockingStream").Construct<object>([model])`;
its local `Invoke`/`Invoke<T>` pair is deleted and the call sites read
`stream.Invoke<bool>("AcquireLock")` |
| `Util/SystemInfoTest.cs` | 4 | `Invoke<bool>(nameof(IsAndroidCore))` and
`Invoke<bool>(nameof(IsMissingConfigurationSystem), [exception])`,
`GetPropertyValue<bool>("IsAndroid")`, and the configuration latch as a private
static `AppSettingsUnavailable` property over `GetFieldValue<bool>` and
`SetFieldValue`, which replaces the `FieldInfo` returning helper |
| `Appender/FileAppenderMutexNameTest.cs` | 2 |
`Invoke<string>(nameof(MutexNameForPath), [path, suffix])`; the arity picks the
overload |
| `Util/AotCompatibilityTest.cs` | 2 | `GetFieldValue<Dictionary<string,
ConverterInfo>>("_sGlobalRulesRegistry")` for `GlobalRules`,
`NonPublicMethod(name, signature)` for `Parameter` |
| `Appender/EventLogAppenderTest.cs` | 2 |
`typeof(EventLogAppender).Invoke<string>(nameof(PrepareEventText), [rendered,
maxSize])` and `appender.Invoke<int>(nameof(GetMaxMessageSize))` |
| `Appender/UdpAppenderTest.cs` | 1 |
`appender.Invoke<byte[]>(nameof(GetDatagramBytes), [message])`; the wrapper is
renamed from `Encode` so the name matches |
| `Appender/ContentEscapeTest.cs` | 1 |
`Assembly.NonPublicType(...).Invoke<string>(nameof(EscapeUnpairedSurrogates),
[message])` |
| `Core/LevelMappingTest.cs` | 1 |
`mapping.Invoke<List<LevelMappingEntry>>("SortEntries")` |
| `Core/LoggingEventTest.cs` | 1 |
`typeof(LoggingEvent).Invoke<string>(nameof(ReviseThreadName), [name])` |
| `Core/UserNameFixingTest.cs` | 1 | `GetFieldValue<string?>` /
`SetFieldValue` around the `try`/`finally`, which stays |
| `Util/TransformTest.cs` | 1 | `NonPublicMethod(nameof(CountSubstrings))`
inside the existing `Delegate.CreateDelegate` |
| `Config/XmlConfiguratorTest.cs` | 1 |
`typeof(XmlConfigurator).Invoke("InternalConfigure", [repository,
getConfigSection])`; all four overloads take two parameters, so the argument
types pick it |
Two files keep reflection visible on purpose, because the member is the
subject of the assertion,
and they use the lookup half of the API rather than the invoke half:
- `LocalSyslogAppenderTest.TheIdentityHandleBelongsToTheProcess` asserts
`field.IsStatic`, and
`TheSyslogDeclarationPassesNoVariadicArgument` asserts the parameter types
- `AotCompatibilityTest` asserts attributes on `MethodInfo` and
`ParameterInfo`
Drop the now unused `using System.Reflection;` from every file that no
longer names a reflection
type, and keep it where one is still declared.
## Changelog
None. Test only, nothing user visible.
## Verification
```
dotnet build src/log4net.sln # 0 warnings
dotnet test src/log4net.Tests/log4net.Tests.csproj -f net10.0 # same
count, same passes
```
- Record the test and pass count **before** the first edit and compare
after. A refactor that
silently stops exercising something is the one real risk: 421 passed, 0
failed, both before and
after; solution and net462 both build with 0 warnings.
- `grep -rn "BindingFlags" src/log4net.Tests` must come back with only
`ReflectionExtensions.cs`.
That is the mechanical check that no site was missed.
- Rename one private member in `src/log4net` by hand, confirm the failure
names the member and the
test, then put it back. Renaming `LocalSyslogAppender.EscapePercent` fails
5 tests with
`log4net.Appender.LocalSyslogAppender.EscapePercent no longer exists -
update this test along
with it.`
- **net462 builds here, it just cannot run.** `log4net.Tests.csproj:8` drops
the TFM off Windows,
but a command line global property overrides that with no file edit:
`dotnet build src/log4net.Tests/log4net.Tests.csproj
-p:TargetFrameworks=net462 -f net462`,
0 warnings. C# 14 extension blocks lower to plain static methods with no
new BCL dependency, so
the five argument `GetMethod` choice is checkable locally. Running the
net462 tests still needs
Windows or CI.
- Windows and macOS coverage comes from CI, since several fixtures are
`[Platform("Win")]` or
Linux only.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
