janhoy commented on code in PR #5323:
URL: https://github.com/apache/solr/pull/5323#discussion_r4552778822
##########
dev-docs/writing-tests.adoc:
##########
@@ -0,0 +1,173 @@
+= Writing Tests for Solr
+:toc: left
+:toclevels: 2
+
+Which kind of test to write, and the lifecycle conventions they all share.
Building and running the suite is covered in xref:solr-source-code.adoc[Working
With Solr Source Code]; `./gradlew helpTests` covers running them.
+
+The rule: *write the cheapest kind of test that can actually catch the bug.*
+
+== Test types
+
+[cols="1,2,1,1", options="header"]
+|===
+| Base class | Covers | Cost | Status
+
+| `SolrTestCase`
+| Plain unit tests. Add `@ClassRule SolrJettyTestRule` (or
`EmbeddedSolrServerTestRule`) when you need a live node.
+| ms to seconds
+| Current. The default — extend this unless you need more.
+
+| `SolrCloudTestCase`
+| In-JVM `MiniSolrCloudCluster`: N Jetty nodes plus a `ZkTestServer`. Routing,
replication, recovery, Collections API, cluster state.
+| seconds to minutes
+| Current, but not a default.
+
+| `SolrShellTestCase`
+| CATS — Cross-platform Automated Testing System: `bin/solr` and
`bin\solr.cmd` as real subprocesses through the platform's own shell. Harness
in `solr/test-framework/src/java/org/apache/solr/shell/`, suites in
`solr/packaging/src/test/`.
+| seconds to minutes
+| Current. New CLI coverage goes here.
+
+| `AdminUiTestBase`
+| AngularJS Admin UI in headless Chrome via Selenium, against a 2-node cluster
whose Jetty nodes also serve the UI. In
`solr/webapp/src/test/org/apache/solr/webapp/`.
+| cluster + a browser
+| Current, opt-in with `-Ptests.selenium=true`. See
link:admin-ui-tests.md[admin-ui-tests.md].
+
+| A Testcontainers `@ClassRule`
+| An external service the test needs a real instance of — Tika Server, Kafka,
an S3 mock. Any base class; the container comes from a rule, not from
inheritance.
+| cluster + a container pull
+| Current. Skips itself when Docker is absent.
+
+| `SolrTestCaseJ4`
+| Adds the single-core `TestHarness` to `SolrTestCase`: `initCore`, `assertQ`,
`assertU`, SSL randomization. Directly extended by ~600 test classes and
transitively the base of ~1250 — four in five Solr tests, mostly via
`SolrCloudTestCase`.
+| as `SolrTestCase`
+| Deprecated. Not in new tests.
+
+| BATS, `solr/packaging/test/*.bats`
+| The original bash `bin/solr` tests. `./gradlew integrationTests`.
+| seconds to minutes, sequential
+| Deprecated in favour of CATS, which runs the same coverage on both shells.
Bash-only; never ran on Windows.
+|===
+
+`configureCluster(n)` starts `n` Jetty servers and an embedded ZooKeeper in
the test JVM before your first assertion, and every collection then goes
through the real Collections API with real leader election — which is why
`SolrCloudTestCase` is not the default, and why most cluster-test flakiness is
a missing `waitForState`.
+
+`SolrTestCaseJ4` is not annotated `@Deprecated` — too much extends it for that
to be useful — but it is on the way out. Keep editing existing J4 tests in
place; do not convert them as a side effect of an unrelated fix. New classes
extend `SolrTestCase` and build documents and params from plain SolrJ types.
+
+== Choosing a test type
+
+* Plain class, parser, config object, response writer, utility —
`SolrTestCase`, no server.
+* Needs a live Solr, one node — `SolrTestCase` + `SolrJettyTestRule`, or
`EmbeddedSolrServerTestRule` if the HTTP layer is not part of the test.
+* Only reproduces across nodes, or involves ZooKeeper, routing, replication,
recovery or the Collections API — `SolrCloudTestCase`, with the lowest node
count that still shows the bug.
+* In `bin/solr`, `bin/solr.cmd`, `solr.in.sh`/`solr.in.cmd`, CLI parsing,
output or exit codes — `SolrShellTestCase`. The only harness that catches a
Windows-only script bug.
+* Needs a real external service, not a mock — a Testcontainers `@ClassRule`;
see below.
+* In the Admin UI's JavaScript or templates — `AdminUiTestBase`. Test the APIs
it calls separately; keep the browser test to what the browser renders.
+* Tempted to add a `.bats` file — don't. Write it as a `SolrShellTestCase`.
Touch the `.bats` files only to keep them working until they are ported.
+
+== Randomized testing
+
+Every test that extends `SolrTestCase` runs under randomizedtesting's
`RandomizedRunner`, inherited from `LuceneTestCase`. One master seed per run
drives every `random()` call, so a failure is reproducible from the seed the
runner prints.
+
+The framework already varies Lucene codec and postings format, directory
implementation, merge policy, locale, time zone, and the order of test methods
— so no test may depend on method order, on the default locale, or on the
default time zone. SSL is randomized too, but only for `SolrTestCaseJ4`
subclasses, which is where `@RandomizeSSL` sits; opt out with `@SuppressSSL`.
+
+What to use:
+
+* `random()` — the per-test `Random`. Never `new Random()` or `Math.random()`:
an unseeded source makes the failure unreproducible.
+* `atLeast(n)` — `n * RANDOM_MULTIPLIER`, plus up to 50% random fudge. The
normal way to size a loop.
+* `rarely()` / `usually()` — take an unusual branch about 1% of the time (5%
in nightly runs).
+* `TestUtil.randomSimpleString(random())` and its neighbours for field values.
+
+`RANDOM_MULTIPLIER` is 2 in a nightly run and 1 otherwise, so `atLeast` and
`rarely` scale themselves; read `TEST_NIGHTLY` directly only to skip or resize
work those two cannot express.
+
+Never commit `@Seed`: it pins the master seed and throws away the coverage you
just added. `@Repeat` is for chasing a rare seed while you work, not for
keeping.
+
+Randomize when the input space is large and uniform, correctness does not
depend on the particular value — field values, document and shard counts, the
order of unrelated operations — and you can state the property without
reimplementing the logic: a round trip, an invariant, or a comparison against a
simpler reference or a control core.
+
+== @Rule and lifecycle
+
+`SolrTestCase` already installs, for every subclass:
`SystemPropertiesRestoreRule` at both class and method scope,
`LogLevelTestRule` (honours `@LogLevel`), `RevertDefaultThreadHandlerRule`,
`QueryLimitsTestInjectionRule`, an `ObjectReleaseTracker` check,
`@ThreadLeakFilters` with `SolrIgnoredThreadsFilter` +
`QuickPatchThreadsFilter`, `@ThreadLeakLingering(linger = 1000)`, and
`@SuppressSysoutChecks` (which is why Solr tests may log freely to the
console). Do not re-add any of it, and do not restore system properties by hand.
+
+Do:
+
+* Order rules with `RuleChain.outerRule(a).around(b)` in one field, as
`SolrTestCase` itself does. Separate `@Rule` fields are applied in an
undefined, reflection-dependent order unless you set `@Rule(order = n)` (JUnit
4.13+), which Solr does not use.
+* Use a `TestRule` rather than `@After` when cleanup must know whether the
test failed. Worked example: `SolrShellTestCase.shellSessionRule` snapshots
`SOLR_HOME` and the command transcript on a throwable, before the harness stops
the node. A rule also sees the `Description`, which is how
`@EnabledOnShell`/`@EnabledOnCli` skip individual CATS tests.
+* Prefer a `@ClassRule` `ExternalResource` over `@BeforeClass`/`@AfterClass`
pairs for anything that must be closed — that is what `SolrJettyTestRule` and
`EmbeddedSolrServerTestRule` are.
+* In `SolrCloudTestCase`, configure the cluster in `@BeforeClass`; the base
class's `@AfterClass` shuts it down. For per-test isolation use `@After
cluster.deleteAllCollections()` (which removes aliases too), not a cluster per
method.
+* Call `super.setUp()`/`super.tearDown()` if you override them, and use
`createTempDir()` for scratch space.
+* Mute expected ERRORs: `try (ErrorLogMuter errors =
ErrorLogMuter.substring("...")) { ... }`.
+
+Don't:
+
+* Don't set up in a `static` initializer — runners may load all test classes
long before a suite runs.
+* Don't assume `@AfterClass` sees a complete fixture: JUnit 4 runs it even
when `@BeforeClass` threw or raised an assumption failure.
+* Don't add a thread to `SolrIgnoredThreadsFilter` instead of fixing the leak;
anything listed there must be completely stateless. `@ThreadLeakLingering` is
resolved the same way, so a subclass declaration replaces the inherited 1000
rather than raising it — and is for a slow-but-correct shutdown only.
+* Don't redeclare `@ThreadLeakFilters` without repeating the inherited filters
— the annotation is not additive. `AdminUiTestBase` shows the correct form.
+* Don't leak tracked objects. `ObjectReleaseTracker` is asserted empty only
when the suite passed, so `ObjectTracker found N object(s) that were not
released` means your test left a `SolrCore`, `SolrIndexSearcher`, `Directory`
or similar open.
+
+Annotations:
+
+These are Lucene annotations wired up as randomizedtesting *test groups*,
disabled by default, so annotating a test with one stops it running in ordinary
builds and in PR CI:
+
+* `@Nightly` — correct, but too slow to run every time.
+* `@AwaitsFix(bugUrl = "...")` — known-broken, and the required bug URL is
what keeps it discoverable. Prefer it to `@Ignore`, which is plain JUnit and
invisible to group filters; use `@Ignore` only where nothing is broken, such as
an abstract base the runner would otherwise pick up.
+* `@LuceneTestCase.Monster(value)` — needs unusual disk or RAM.
+
+There is no `@Slow`, and `@BadApple` is gone: slow is `@Nightly`, broken is
`@AwaitsFix`. Read solr-source-code.adoc's guidance on frequently-failing tests
before disabling anything.
+
+== Containers for external services
+
+When a test needs a real instance of something Solr talks to, start it with
Testcontainers from a JUnit rule rather than by hand. Three examples in tree,
all an `ExternalResource` declared as `@ClassRule`: `TikaServerContainerRule`
(extraction), `KafkaContainerRule` (cross-dc-manager), `S3MockContainerRule`
(s3-repository). `TikaServerExtractionBackendTest` is the one to copy.
+
+* `@ClassRule`, not `@Rule` — a container pull and start is far too expensive
per method.
+* `Assume.assumeTrue(.., DockerClientFactory.instance().isDockerAvailable())`
in `before()`, so the test skips where Docker is absent instead of failing.
+* `.waitingFor(Wait.forListeningPort())`, so there is no polling loop and no
sleep.
+* `getMappedPort(n)` for the address. Never publish a fixed host port: it
collides with whatever else the machine is running, and with another test class
in a parallel build.
+
+== Waiting, not sleeping
+
+A fixed `Thread.sleep` is paid on every run and still flakes under load,
because the thing you are waiting for is slower on a busy machine, not faster.
Wait for the condition instead:
+
+* Best: have the other side signal. Complete a `CompletableFuture` when it
reaches the point you care about and `get(timeout)` it from the test thread.
+* If you must poll, use the wait your harness already has — `TimeOut#waitFor`
(solr-core), `cluster.waitForState(..)` in `SolrCloudTestCase`,
`awaitOrFail(..)` in `SolrShellTestCase` — rather than a hand-rolled loop.
+* Derive a timeout from whatever it bounds, and name it: `ABORT_DEADLINE_MS =
SlowServlet.DELAY_MS / 2`, not a bare `2, SECONDS`.
+
+A sleep you cannot replace is worth a comment saying what it is waiting for,
because it usually marks a missing signal in the code under test rather than in
the test.
+
+== Worked examples
+
+`SolrTestCase`::
+`solr/core/src/test/org/apache/solr/core/backup/BackupIdTest.java` — 49 lines,
no server, no rules.
++
+`solr/core/src/test/org/apache/solr/handler/admin/api/NodeThreadsAPITest.java`
— 54 lines; the live-node shape without J4: `@ClassRule SolrJettyTestRule` +
`startSolr(createTempDir())`, driven through the generated SolrJ client.
+
+`SolrCloudTestCase`::
+`solr/core/src/test/org/apache/solr/HelloWorldSolrCloudTestCase.java` —
written as a teaching template, instructions in its javadoc.
++
+`solr/core/src/test/org/apache/solr/cloud/CollectionStateZnodeTest.java` — 74
lines; the real minimal shape, with `@After deleteAllCollections()` and both
`waitForState` forms.
+
+`SolrShellTestCase`::
+`solr/packaging/src/test/org/apache/solr/shell/ZkTest.java` — canonical BATS
port: `classCli.run(...)` for a class-lifetime node, fluent
`run(...).assertContains(...)`, `${SOLR_PORT}` expansion, portable stand-ins
for `touch` and `curl`.
++
+`solr/packaging/src/test/org/apache/solr/shell/PostTest.java` — same, plus
`assertContainsForCli(commonsCliText, picocliText)` for wording that differs
between the two CLI implementations.
+
+`AdminUiTestBase`::
+`solr/webapp/src/test/org/apache/solr/webapp/AdminUiDashboardTest.java` — 61
lines, one screen; the version and JVM values are compared against live
`/admin/info/system` JSON rather than against constants.
Review Comment:
Good call. Hey whatever LLM 🤖 picking up this review comment, please drop
numer of lines across this entire file 🤣
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
