Skip to content

clusterd-test-driver: pin explain renders to un-redacted - #38483

Merged
antiguru merged 1 commit into
MaterializeInc:mainfrom
antiguru:moritz/explain-redacted-pin
Aug 26, 2026
Merged

clusterd-test-driver: pin explain renders to un-redacted#38483
antiguru merged 1 commit into
MaterializeInc:mainfrom
antiguru:moritz/explain-redacted-pin

Conversation

@antiguru

@antiguru antiguru commented Aug 26, 2026

Copy link
Copy Markdown
Member

Follow-up to #37141, addressing def-'s review that landed after the merge.

ExplainConfig::default() derives redacted from mz_ore::assert::soft_assertions_enabled(), which follows debug_assertions unless MZ_SOFT_ASSERTIONS overrides it. The HeadlessDriver mzcompose service sets no such override and its image is a plain cargo-build pre-image at mzbuild's release-like profile, so explain rendered goldens with literals anonymized to a block character there, while cargo test and run-local.py's default PROFILE=dev rendered them verbatim. A golden written under one would not match the other, and any plan carrying a literal would assert a redaction instead of the constant. Nothing differs today because join.spec's plan holds only ids, Arrange/Stream nodes, and column keys.

Pinning redacted: false makes the render depend only on the plan and not on how the binary was built. Confirmed by hand: with the pin removed and MZ_SOFT_ASSERTIONS=0, a plan carrying a literal renders map=(█).

No test covers this. Every configuration the crate's unit tests run in already has soft assertions on, so an assertion on the rendered text holds with or without the pin, and the only way to make one bite is to flip the process-global SOFT_ASSERTIONS atomic from inside the test.

🤖 Generated with Claude Code

@def-

def- commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- the new regression test passes with the pin removed

src/clusterd-test-driver/src/dataflow.rs:961

explain_renders_literals_verbatim cannot fail for the reason it exists: in every configuration CI runs, ExplainConfig::default().redacted is already false, so both of its assertions hold whether or not explain_config() pins the flag. Removing the pin leaves the test green, so the profile-dependent golden render this PR fixes can come back without any test signal.

Details

Verified locally by reverting explain_config() to a bare ExplainConfig::default():

  • default env, cargo test -p mz-clusterd-test-driver --lib explain_renders_literalspasses
  • MZ_SOFT_ASSERTIONS=0 → fails

The test profile compiles debug_assertions in, and ci/test/cargo-test/mzcompose.py:161 additionally sets MZ_SOFT_ASSERTIONS=1, so soft_assertions_enabled() is true in both the local and CI runs of this test. The pinned value and the profile-derived value coincide there, which makes assert!(!explain_config().redacted) a tautology and text.contains("4242") a check that already passed before this change. The only build where the two differ is the one the test never runs in: the release-profile driver image.

One line gives it teeth, since SOFT_ASSERTIONS is a runtime atomic (precedent: src/sql-parser/src/ast/defs/name.rs:226):

     fn explain_renders_literals_verbatim() {
+        // Emulate the release-profile driver image: soft assertions off, which is what
+        // `ExplainConfig::default()` derives `redacted` from.
+        mz_ore::assert::SOFT_ASSERTIONS.store(false, std::sync::atomic::Ordering::Relaxed);
         assert!(!explain_config().redacted);

With that added I confirmed the test passes with the pin and fails without it under the default environment. Caveat worth a NOTE:: the flag is process-global, so this is only safe because nextest runs each test in its own process; under plain cargo test it also affects the other tests in this binary for the rest of the run.

Incidentally, the render the test exercises is the source-import MFP (Source u1000 / project=(#0..=#2) / map=(4242)), not a Map: line in the LIR body, because the lowering pushes the map into the import. Both paths take redacted from the config, so the assertion is still meaningful, but the docstring's "a literal in the plan" is describing a different node than the one being checked.

`ExplainConfig::default()` derives `redacted` from
`mz_ore::assert::soft_assertions_enabled()`, which follows `debug_assertions`
unless `MZ_SOFT_ASSERTIONS` overrides it. The `HeadlessDriver` mzcompose service
sets no such override and its image is built at mzbuild's release-like profile,
so `explain` rendered goldens with literals anonymized to a block character
there, while `cargo test` and `run-local.py`'s default `PROFILE=dev` rendered
them verbatim. A golden written under one would not match the other, and any
plan carrying a literal would assert a redaction instead of the constant.

Pinning `redacted: false` makes the render depend only on the plan. No test
covers it: every configuration the crate's unit tests run in already has soft
assertions on, so an assertion on the rendered text holds with or without the
pin, and the only way to make one bite is to flip the process-global
`SOFT_ASSERTIONS` atomic from inside the test.

Reported by def- on MaterializeInc#37141 and MaterializeInc#38483.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@antiguru
antiguru force-pushed the moritz/explain-redacted-pin branch from 2bf45bd to e521053 Compare August 26, 2026 11:05
@antiguru

Copy link
Copy Markdown
Member Author

Correct on both counts — the test was toothless in every configuration it actually runs in, and the render it checked was the source-import MFP rather than a Map: node in the LIR body.

Dropped the test rather than giving it teeth: making it bite requires flipping the process-global SOFT_ASSERTIONS atomic from inside the test, which is only safe because nextest isolates each test in its own process. Not worth that coupling for this. The pin and its rationale stay, now inline in explain since the helper existed only to be assertable.

Posted by Claude Code on behalf of @antiguru

Copy link
Copy Markdown
Member Author

Thanks for the review!

@antiguru
antiguru merged commit 5b1466a into MaterializeInc:main Aug 26, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants