Re: [I] Prevent users from overriding test framework setup/teardown functionality [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin commented on issue #1087:
URL: https://github.com/apache/lucenenet/issues/1087#issuecomment-4860279055

   I had Claude Code investigate this and write some (possibly throwaway) code to confirm this was possible with NUnit 4. The branch is visible here: https://github.com/paulirwin/lucene.net/tree/spike/1087-nunit4-hook-ordering
   
   Its conclusion follows.
   
   ## Spike results
   
   Both runs produced the same hook ordering, and crucially the **NO BASE** run (overrides that never call `base`) shows the hooks firing identically to the WITH BASE run:
   
   ```
   HOOK.BeforeEverySetUp[OneTime]      <- framework wraps BEFORE subclass OneTimeSetUp
     SUB/NOBASE.OneTimeSetUp
   HOOK.AfterEverySetUp[OneTime]
   HOOK.BeforeEverySetUp[Test]         <- wraps BEFORE subclass SetUp, per test
     SUB/NOBASE.SetUp
   HOOK.AfterEverySetUp[Test]
   HOOK.BeforeTest
     SUB/NOBASE.TestA
   HOOK.AfterTest
   HOOK.BeforeEveryTearDown[Test]
     SUB/NOBASE.TearDown
   HOOK.AfterEveryTearDown[Test]
   HOOK.BeforeEveryTearDown[OneTime]
     SUB/NOBASE.OneTimeTearDown
   HOOK.AfterEveryTearDown[OneTime]    <- framework wraps AFTER subclass OneTimeTearDown
   ```
   
   ### What this proves
   
   1. **Ordering guarantee holds.** A class-scoped `ExecutionHookAttribute` runs `BeforeEverySetUpHook` *before* the subclass's `OneTimeSetUp`/`SetUp`, and `AfterEveryTearDownHook` *after* the subclass's `OneTimeTearDown`/`TearDown`. That's precisely the wrap the original issue's static-method approach was trying to achieve.
   
   2. **Hooks are independent of `base` calls.** This is the headline: when the subclass overrides all four methods and *never calls `base`*, the hooks still fire in the exact same positions. So if we move the framework's mandatory work (`ClassEnvRule.Before/After`, temp-file cleanup, `RandomizedContext.DisposeResources`, failure-repro reporting) into a hook, a user can no longer disable it by forgetting `base`. This is the core goal of #1087, achieved **structurally, without analyzers.**
   
   3. **One-time vs per-test is cleanly distinguishable** via `HookData.Context.Test.IsSuite` (`[OneTime]` vs `[Test]` in the output).
   
   4. **`AfterEverySetUp` / `BeforeEveryTearDown` fire per-phase**, giving fine-grained wrap points if we want them.
   
   ### Caveat to note
   This ran through the internal `TestBuilder`/`WorkItem` harness, which confirms hooks fire there (important, since our TestFramework self-tests use it). It would still be worth one confirmation run under the real `dotnet test` runner during implementation, but the harness path exercising hooks is actually the stronger result for our test-of-tests infrastructure.
   
   ### Verdict
   **Go with Execution Hooks.** The ordering @NightOwl888 was worried about ("if NUnit doesn't call the base class first... we may still need to report incorrect usage") is satisfied, the framework hook wraps the subclass on both ends. The "must call `base`" Roslyn analyzer (rule #2 in the issue) becomes unnecessary; framework behavior is now unskippable by construction. Rules #1 (ban native `[SetUp]`/`[TearDown]`) and #3 (ordering warning) become optional polish rather than load-bearing.
   


-- 
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: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org