Repository navigation
acceptance: give title its own trailing newline - #7003
Draft
janniklasrose wants to merge 1 commit into
Draft
janniklasrose wants to merge 1 commit into
janniklasrose wants to merge 1 commit into
Conversation
Follow-up to the review thread on #6957. `title` printed `\n=== <label>` with no trailing newline, relying on a following `trace` (which prepends `\n`) to terminate the header line. When a `title` was instead followed by non-`trace` output the header got mashed onto the next line (e.g. `=== Recorded request bodies{`), and authors worked around the missing newline inconsistently: a bare `echo` after the title, or a literal `\n` appended to the label. Make `title` self-contained by printing its own trailing newline. To keep the common `title`+`trace` output tight (no blank line between the header and the command), `trace` drops its leading newline when it directly follows a `title`; a `trace` after any intervening output still gets its usual separator. This is tracked with an `_after_title` flag that `title` sets, `trace` reads and clears, and a `DEBUG` trap clears before any other top-level command. The trap needs `set -T` because the main script runs in a subshell, and a `FUNCNAME` guard keeps it from firing inside the helpers themselves. Remove the now-redundant workarounds (bare `echo`/`echo ""` after a title, trailing `\n` in labels) and regenerate the acceptance goldens. Co-authored-by: Isaac <no-reply@databricks.com>
This branch has not been deployed
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
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.
Follow-up to the review thread on #6957 (#6957 (comment)).
Problem
The acceptance-test
titlehelper printed\n=== <label>with no trailing newline, relying on a followingtrace(which prepends\n) to terminate the header line. This had two consequences:titlewas followed by non-traceoutput, the header got mashed onto the output, e.g.=== Recorded create-branch request bodies{.echoafter the title (139 occurrences) or a literal\nappended to the label (~359 occurrences).Change
titlenow prints its own trailing newline, so it is self-contained and the mashing is fixed. To keep the commontitle+traceoutput tight (no blank line between the=== headerand the>>> command),tracedrops its leading newline when it immediately follows atitle; atraceafter any intervening output still gets its usual blank-line separator.The "immediately follows a title" state is tracked by an
_after_titleflag:titlesets it,tracereads and clears it, and aDEBUGtrap clears it before any other top-level command. The trap requiresset -Tbecause the harness runs the main script in a subshell (readMergedScriptContentsinacceptance_test.go), and aFUNCNAMEguard keeps it from firing inside the helper functions themselves.The now-redundant workarounds are removed and all acceptance goldens regenerated.
Net effect on
output.txttitle+traceblocks: unchanged.titlefollowed by non-traceoutput: no longer mashed onto the header line.\n, bareecho): removed, so these blocks match the tight majority.Testing
./task testregenerated via-update; full suite re-run without-updatepasses. The only failures observed locally are pre-existing/environmental and unrelated to this change:bundle/deploy/mlops-stacks(needs network to clone the upstream template; itsoutput.txtis unchanged here) andbundle/apps/job_permissions(documented-flaky deploy-order output inout.after_first_deploy.txt, which this change does not touch).This pull request and its description were written by Isaac.