Prevent users from overriding test framework setup/teardown functionality, #1087 - #1427
Draft
paulirwin wants to merge 4 commits into
Draft
Prevent users from overriding test framework setup/teardown functionality, #1087#1427paulirwin wants to merge 4 commits into
paulirwin wants to merge 4 commits into
Conversation
…ase calls, apache#1087 Moves the mandatory framework work out of the virtual OneTimeSetUp/ OneTimeTearDown/TearDown methods into static __OneTimeSetUp/ __OneTimeTearDown/__TearDown methods declared at the root of the LuceneTestCase hierarchy. NUnit runs base-level setup methods first and base-level teardown methods last, and statics cannot be overridden, so subclasses can no longer disable the framework's environment setup, temp-file cleanup, resource disposal, or failure-repro reporting by failing to call the base method. The virtual methods remain as empty extension points for subclasses. NUnit 4 Execution Hooks were evaluated first (see spike) but cannot be used: ExecutionHookAttribute.Wrap is non-overridable and throws when another command wrapper (e.g. LuceneDelegatingTestCommand from [Slow]) is already applied, and the hook registration APIs are internal to NUnit. Suite-level ITestAction.AfterTest was also evaluated, but NUnit runs it before the [OneTimeTearDown] methods, not after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Asserts the cross-level ordering the static lifecycle method design depends on: setup methods run base-level-first and teardown methods run derived-level-first (so the framework's root-level statics bracket all user code); an overridden virtual lifecycle method runs once, at the override's declaring level (the "relocation" behavior described in the issue); and same-named static lifecycle methods at multiple levels (the Java Lucene beforeClass/afterClass convention) all run, base-first for setup and derived-first for teardown. Within-level ordering of multiple lifecycle methods is unspecified by NUnit and deliberately not asserted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pache#1087 Adds the missing __SetUp counterpart to the framework-owned static lifecycle methods. The testCaseThread capture that IsTestThread depends on still lived in the skippable virtual SetUp(), so fixtures that override SetUp() without calling base (TestBlockJoinValidation, TestBufferedCharFilter and LookupBenchmarkTest do) left IsTestThread false on the test thread, silently disabling logic gated on it. It now runs in FrameworkSetUp() from the static __SetUp(). Also from review: - Makes the four __ lifecycle methods protected rather than public, so they stay out of the shipped public API. NUnit accepts protected setup/teardown methods (TestSuite.CheckSetUpTearDownMethods allows IsPublic || IsFamily), so the doc claim that they must be public was wrong and has been corrected. - Adds [CLSCompliant(false)] to them; the leading underscores otherwise emit CS3008 warnings in this CLSCompliant(true) assembly. - Documents on each empty virtual that framework work must not be added back: a non-overridden virtual runs at the same level as its static counterpart, where NUnit does not define the relative order. - Extracts LifecycleAssert.AssertBefore so both lifecycle test classes share one ordering assertion that verifies each event was recorded; the bare IndexOf comparison passed vacuously when an event was absent. - Replaces comments describing a run-once guard that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hods, apache#1087 These commented-out ports track work that would belong with the test framework's own per-test setup/teardown, not with the now-empty overridable extension points. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent users from overriding test framework setup/teardown functionality
Fixes #1087
Description
See #1087 and discussion for context. Currently, the setup/teardown methods are virtual and can be overridden by users. If a user does this and does not call the base method (or calls it out of order), unexpected things could happen.
I spent a good deal of time evaluating NUnit 4's Execution Hooks feature for this. It doesn't quite work cleanly for what we need without a few modifications (which I'm currently discussing with their team over at nunit/nunit#5368). So to unblock this issue and avoid having to wait for those things to be fixed, I adopted the suggestion from that thread of using non-virtual methods to work around the problem. That means that this PR does not require NUnit 4 to be merged first (although not being able to use EHs right now does not invalidate the other benefits of upgrading NUnit, who are about to release v5 soon).
In NUnit, if you give a test method a unique name from your overridable method (like prefixing it with underscores like I did here), it will run separate from the other method's inheritance stack, starting with the base level first. This solves the problem by moving the framework setup/teardown into dedicated static methods. Static methods are inherently non-virtual (can't be overridden), and because they're on the base class, they run when we need them to in the framework. Meanwhile, this PR retains the existing virtual SetUp/TearDown methods to allow for derived classes to override. The static methods are marked editor browseable never so that they do not show up in intellisense, and are non-CLS Compliant because of the leading double underscores. Finally, they are
protectedto further shield them from prying eyes from the outside.This is not as clean of a solution as execution hooks (a derived class could still call i.e.
__SetUp(), if they manually type it without IDE help), but after many hours of analysis, it's probably as good as we're going to get without them.Since the original methods are retained, this is not a breaking change from a compilation standpoint. There are some slight changes in behavior that would likely not affect anyone in the real world. Because these are fringe concerns and currently usage of our test framework is low, I don't think this warrants the "breaking change" label:
DisposeAfterTest-registered resources are now disposed after all[TearDown]methods rather than at thebase.TearDown()call position. A fixture doingbase.TearDown(); dir.Dispose();now gets a different disposal order.ClassEnvRule.Before()(randomized culture/codec/timezone/similarity) now runs before all user one-time setup code, including code an override ran beforebase.OneTimeSetUp()and any uniquely-named[OneTimeSetUp]methods at intermediate levels.Several unit tests were added to help enforce the invariant that we must have the correct order when dealing with lifecycle events.
AI: Claude Code (Opus 4.8 and 5; Fable 5) helped me work through this analysis and wrote most of this code.
Next Steps
If there are any recommended changes from a maintainer here, I kindly request going ahead and making the changes yourself ("allow edits" is enabled on this PR). I've spent more time on this issue than I would have liked (or is perhaps even warranted), and I have run out of energy on it. I would greatly appreciate the collaboration of your commits or at least GitHub code suggestion blocks in comments that can be easily applied.