diff --git a/.agents/skills/pull-request-writing/SKILL.md b/.agents/skills/pull-request-writing/SKILL.md new file mode 100644 index 00000000..618d30eb --- /dev/null +++ b/.agents/skills/pull-request-writing/SKILL.md @@ -0,0 +1,15 @@ +--- +name: pull-request-writing +description: "How to title and describe a DxMessaging pull request: short + Simplified Technical English, one idea per sentence, and a body that answers + why the problem mattered, what the change does, and how we know it is correct. + Bans jargon, filler, status narration, and session diaries. Use when opening + or editing a pull request, writing its title, rewriting a description a + reviewer found hard to read, or deciding what evidence belongs in the body." +--- + + + +Canonical instructions: [`.llm/skills/pull-request-writing/SKILL.md`](../../../.llm/skills/pull-request-writing/SKILL.md) + +Read that file and follow it. Supporting detail is in the sibling `references/` directory. diff --git a/.claude/skills/pull-request-writing/SKILL.md b/.claude/skills/pull-request-writing/SKILL.md new file mode 100644 index 00000000..618d30eb --- /dev/null +++ b/.claude/skills/pull-request-writing/SKILL.md @@ -0,0 +1,15 @@ +--- +name: pull-request-writing +description: "How to title and describe a DxMessaging pull request: short + Simplified Technical English, one idea per sentence, and a body that answers + why the problem mattered, what the change does, and how we know it is correct. + Bans jargon, filler, status narration, and session diaries. Use when opening + or editing a pull request, writing its title, rewriting a description a + reviewer found hard to read, or deciding what evidence belongs in the body." +--- + + + +Canonical instructions: [`.llm/skills/pull-request-writing/SKILL.md`](../../../.llm/skills/pull-request-writing/SKILL.md) + +Read that file and follow it. Supporting detail is in the sibling `references/` directory. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index c5d1edc1..ffa895c9 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,12 +1,24 @@ -## Description + - +## Why -## Related Issue + + +## What changed - + + +## How we know + + + +## Related Issue -Fixes # +Closes # ## Type of Change diff --git a/.llm/context.md b/.llm/context.md index a9ffd068..cb086ed5 100644 --- a/.llm/context.md +++ b/.llm/context.md @@ -17,9 +17,9 @@ discover it. `.llm/` is the single source of truth; the mirrors are generated po -31 skills are registered. See [the skill index](./index.md) for the full table. +32 skills are registered. See [the skill index](./index.md) for the full table. -`allocation-and-leak-coverage`, `api-design-patterns`, `benchmark-methodology`, `cache-eviction`, `changelog-management`, `collection-pooling`, `data-driven-tests`, `dispatch-hot-path`, `documentation-code-samples`, `documentation-style`, `github-access`, `github-workflow-consistency`, `il2cpp-build-configuration`, `markdown-authoring`, `memory-reclamation`, `object-pooling`, `package-publishing`, `serializable-dictionary`, `shell-and-powershell`, `singleton-patterns`, `test-code-quality`, `test-coverage-design`, `test-diagnostics`, `test-failure-investigation`, `test-fixtures-and-cleanup`, `unity-editor-ci`, `unity-editor-conventions`, `unity-licensing`, `unity-mcp-test-loop`, `unity-test-execution`, `value-equality-and-hashing` +`allocation-and-leak-coverage`, `api-design-patterns`, `benchmark-methodology`, `cache-eviction`, `changelog-management`, `collection-pooling`, `data-driven-tests`, `dispatch-hot-path`, `documentation-code-samples`, `documentation-style`, `github-access`, `github-workflow-consistency`, `il2cpp-build-configuration`, `markdown-authoring`, `memory-reclamation`, `object-pooling`, `package-publishing`, `pull-request-writing`, `serializable-dictionary`, `shell-and-powershell`, `singleton-patterns`, `test-code-quality`, `test-coverage-design`, `test-diagnostics`, `test-failure-investigation`, `test-fixtures-and-cleanup`, `unity-editor-ci`, `unity-editor-conventions`, `unity-licensing`, `unity-mcp-test-loop`, `unity-test-execution`, `value-equality-and-hashing` @@ -55,6 +55,9 @@ discover it. `.llm/` is the single source of truth; the mirrors are generated po checkout, commit, branch, fetch, rebase, and push operations. Use `gh` only as the final fallback when neither the connector nor `git` covers the required operation. Preserve this order. - Never commit repository settings that auto-approve chat-invoked terminal commands. +- Write pull request titles and descriptions in short Simplified Technical English: say why the + problem mattered, what the change does, and how you know it is correct. No jargon, no filler, no + session diary. Follow [pull-request-writing](./skills/pull-request-writing/SKILL.md). - Ensure fenced markdown examples are closed and do not swallow real sections (for example `## See Also`). - Before committing, run the relevant formatters/linters yourself (`npm run format:check`, `npm run lint:markdown`, `npm run check:spelling`, `npm run validate:all` as applicable); hooks are the fast backstop, not the first signal. diff --git a/.llm/index.json b/.llm/index.json index e6213fa7..cef4c252 100644 --- a/.llm/index.json +++ b/.llm/index.json @@ -1,5 +1,5 @@ { - "skillCount": 31, + "skillCount": 32, "skills": [ { "name": "allocation-and-leak-coverage", @@ -293,6 +293,19 @@ ".llm/skills/package-publishing/references/unity-analyzer-shipping.md" ] }, + { + "name": "pull-request-writing", + "path": ".llm/skills/pull-request-writing/SKILL.md", + "description": "How to title and describe a DxMessaging pull request: short Simplified Technical English, one idea per sentence, and a body that answers why the problem mattered, what the change does, and how we know it is correct. Bans jargon, filler, status narration, and session diaries. Use when opening or editing a pull request, writing its title, rewriting a description a reviewer found hard to read, or deciding what evidence belongs in the body.", + "metadata": { + "category": "process", + "tags": "pull-request, writing, simplified-technical-english, ste, review, communication" + }, + "lineCount": 97, + "references": [ + ".llm/skills/pull-request-writing/references/pull-request-examples.md" + ] + }, { "name": "serializable-dictionary", "path": ".llm/skills/serializable-dictionary/SKILL.md", @@ -317,7 +330,7 @@ "category": "scripting", "tags": "cross-platform, case-sensitivity, testing, powershell, javascript, ci-cd, linux, windows, macos" }, - "lineCount": 124, + "lineCount": 128, "references": [ ".llm/skills/shell-and-powershell/references/cross-platform-compatibility.md", ".llm/skills/shell-and-powershell/references/powershell-best-practices-part-1.md", diff --git a/.llm/index.md b/.llm/index.md index 845c0608..40070813 100644 --- a/.llm/index.md +++ b/.llm/index.md @@ -2,7 +2,7 @@ -31 skills. Each is an [Agent Skills](https://agentskills.io/specification) +32 skills. Each is an [Agent Skills](https://agentskills.io/specification) directory: `SKILL.md` holds the instructions, `references/` holds supporting detail loaded on demand. | Skill | References | Description | @@ -24,6 +24,7 @@ directory: `SKILL.md` holds the instructions, `references/` holds supporting det | [memory-reclamation](./skills/memory-reclamation/SKILL.md) | 3 | How DxMessaging reclaims empty per-type and per-InstanceId slots through counter-based idle sweeps, IMessageBus.Trim, and DxPools caps, plus the test and documentation duties for any new memory holder. Use when adding a MessageCache field, a dictionary or pool keyed by message type or InstanceId, when slot counts or memory grow over a long session, or when changing DxMessagingRuntimeSettings eviction and buffer settings. | | [object-pooling](./skills/object-pooling/SKILL.md) | 6 | Reuse pooled message objects through Rent/Return instead of allocating a new instance per emit, so dispatch stays allocation-free. Use when adding a message type, seeing GC spikes or a rising allocation rate in the Unity profiler, writing an allocation test, or choosing between a pooled class message and a readonly struct message. | | [package-publishing](./skills/package-publishing/SKILL.md) | 3 | Controlling what ships in the com.wallstop-studios.dxmessaging npm/UPM package: the package.json files allowlist versus .npmignore exclusions, Unity .meta pairing rules for every shipped file and directory, the issue #204 tarball invariants verified against npm pack --json --dry-run, and where RoslynAnalyzer-labeled DLLs must live (Runtime/Analyzers/, never an editor-only asmdef) so the source generator reaches Assembly-CSharp. Use when adding or excluding files from the package, adding a new output directory, seeing CS0315/CS0452 on consumer [Dx*Message] types, or building and verifying the analyzer payload. | +| [pull-request-writing](./skills/pull-request-writing/SKILL.md) | 1 | How to title and describe a DxMessaging pull request: short Simplified Technical English, one idea per sentence, and a body that answers why the problem mattered, what the change does, and how we know it is correct. Bans jargon, filler, status narration, and session diaries. Use when opening or editing a pull request, writing its title, rewriting a description a reviewer found hard to read, or deciding what evidence belongs in the body. | | [serializable-dictionary](./skills/serializable-dictionary/SKILL.md) | 4 | Back a runtime Dictionary with parallel SerializeField lists through ISerializationCallbackReceiver so key/value config is editable in the Unity Inspector and persists in scenes, prefabs, and ScriptableObjects. Use when a SerializeField Dictionary silently stays empty, when parallel key/value lists are drifting out of sync, or when a designer needs to edit a lookup table in the Inspector. | | [shell-and-powershell](./skills/shell-and-powershell/SKILL.md) | 9 | Writing bash and PowerShell that survives CI: set -e error handling for grep/diff/rm, quoting, filename case sensitivity on Linux runners, the PowerShell StrictMode 0/1/many gotcha that requires @()-wrapping every captured result before reading .Count or indexing, here-string quoting, WriteAllText encoding, regex non-greedy versus character-class exclusion, accurate regex comments for the g/i/m/s/u flags, Windows PE-binary stub requirements, cross-drive path containment, and tar archive operands. Use when adding or editing a .sh, .ps1, or Node script, or when a script passes locally and fails on a Linux or Windows runner. | | [singleton-patterns](./skills/singleton-patterns/SKILL.md) | 5 | Implement Unity global managers with RuntimeSingleton for MonoBehaviours that survive scene loads and ScriptableObjectSingleton for Resources-backed config assets, including duplicate handling and quit-time safety. Use when writing a GameManager or AudioManager, when a static Instance field is null after a scene change or on quit, when two copies of a manager exist, or when bootstrapping a manager before the first scene. | diff --git a/.llm/skills/pull-request-writing/SKILL.md b/.llm/skills/pull-request-writing/SKILL.md new file mode 100644 index 00000000..49bacef8 --- /dev/null +++ b/.llm/skills/pull-request-writing/SKILL.md @@ -0,0 +1,97 @@ +--- +name: pull-request-writing +description: "How to title and describe a DxMessaging pull request: short Simplified Technical English, one idea per sentence, and a body that answers why the problem mattered, what the change does, and how we know it is correct. Bans jargon, filler, status narration, and session diaries. Use when opening or editing a pull request, writing its title, rewriting a description a reviewer found hard to read, or deciding what evidence belongs in the body." +metadata: + category: "process" + tags: "pull-request, writing, simplified-technical-english, ste, review, communication" +--- + +# Pull Request Writing + +A pull request is read by a person who was not in the session. They read the title in a list of +titles, and the body once, to decide whether to trust the change. Write for that reader. + +The whole body is written in Simplified Technical English (STE): short sentences, common words, +one idea per sentence, active voice. + +## When to use + +- Opening a pull request, or editing its title or description. +- A reviewer says a description is long, unclear, or hard to follow. +- Deciding which measurements, tables, or logs belong in the body. +- Writing the pull request that closes several issues at once. + +## Rules + +### Title + +- One line, at most 72 characters, plain sentence case. +- Say what the change does, in the words a user would use. Not the mechanism. +- Start with a verb: `Fix`, `Add`, `Remove`, `Speed up`, `Stop`. +- No conventional-commit prefixes, no issue numbers, no branch names, no "and" chains that + join three unrelated things. If the title needs three clauses, lead with the one a reader + cares about most and let the body carry the rest. + +Good: `Keep wrapped Flow Graph rows inside their own box` +Bad: `fix(editor): DxMessagingFlowGraphWindow flexWrap container height resolution on 2021.3` + +### Body shape + +Three short sections, in this order, and nothing else by default: + +1. **Why** - what went wrong for a user, or what they could not do. One short paragraph. +1. **What changed** - what the code does now. A short list if there is more than one part. +1. **How we know** - the test, the measurement, or the run that proves it. + +Add a `Closes #NNN` line for each issue the change closes. Add a fourth section only when the +change leaves something open that a reader must know about. + +### Length + +- Aim for 200 words per block. Stop at 400. +- A pull request closing several issues has one `Why / What changed / How we know` block per + issue, so it is longer overall. The ceiling is per block, not per page. It still does not get a + preamble. +- Move deep evidence to the issue or to a `progress/` record and link it. The body carries the + verdict, not the working. + +### Simplified Technical English + +- One idea per sentence. Under 20 words. +- Active voice, present tense: "The row now grows", not "The row will have been grown". +- One word per meaning. Pick `fix` or `repair`, not both, and use it everywhere. +- Use the plainest word that is still exact: `use` not `utilize`, `so` not `hence`, `before` + not `prior to`, `now` not `at this time`. +- Expand an abbreviation on first use unless it names a file, a type, or a workflow. +- No nested clauses, no dashes carrying a second thought, no sentence that needs re-reading. +- ASCII only, exactly as in [documentation-style](../documentation-style/SKILL.md). + +### Banned + +- Filler and marketing: `comprehensive`, `robust`, `seamless`, `leverage`, `delve`, `simply`, + `just`, `note that`, `it is worth noting`. +- Status narration: `As requested`, `Per the previous session`, `This PR does the following`. +- Session diaries: what was tried and rejected, how long something took, which agent did what. + A rejected approach belongs in the issue only when the next person would otherwise retry it. +- Restating the diff. The file list is already on the page. +- Emoji, headings deeper than `###`, and tables with one row. + +## Verification + +Before you open or update the pull request, read the title and body once as a stranger: + +- Does the title alone say what changed? +- Does the body say why it mattered before it says what you did? +- Is there a sentence over 20 words, or a word a new contributor would look up? +- Is there a claim with no evidence behind it? + +Fix what fails. Do not add words to fix it. + +## See Also + +- [Pull Request Examples](./references/pull-request-examples.md) - a rewrite, an issue-closing + body, and the fourth-section case. +- [documentation-style](../documentation-style/SKILL.md) - the ASCII and banned-phrase rules + this skill inherits. +- [changelog-management](../changelog-management/SKILL.md) - what the same change owes + `CHANGELOG.md`, which is written for users, not reviewers. diff --git a/.llm/skills/pull-request-writing/references/pull-request-examples.md b/.llm/skills/pull-request-writing/references/pull-request-examples.md new file mode 100644 index 00000000..7ec288bb --- /dev/null +++ b/.llm/skills/pull-request-writing/references/pull-request-examples.md @@ -0,0 +1,104 @@ +# Pull Request Examples + +Worked examples for [pull-request-writing](../SKILL.md). Each one shows a real failure mode and +the shorter version that replaces it. + +## A rewrite + +### Before + +> ## Summary +> +> This PR provides a comprehensive fix for the long-standing layout issue in the Flow Graph +> details pane, which has been observed to manifest on Unity 2021.3 LTS but not on newer +> editors. As requested in the previous session, I first investigated whether the root cause +> was related to `align-content` resolution semantics, then determined through experimentation +> that the actual issue is that Yoga does not derive the cross-axis extent of a multi-line flex +> container from its flex lines on that version. Note that this was not caught earlier because +> the local MCP verification loop only runs against a single host editor (6000.4.6f1). Ten +> separate container declarations were audited and it was determined that all of them are +> potentially affected, and so a shared helper was leveraged to seamlessly address the entire +> class of problem in one place, rather than redesigning each container individually. + +Everything wrong with it: 130 words with no verdict, three abandoned investigation steps, a +banned word in almost every sentence, and no evidence. + +### After + +> ## Why +> +> On Unity 2021.3, a wrapping row keeps the height of one line. The extra lines draw outside +> the row, on top of the block beneath it. The Flow Graph details pane has ten such rows, so a +> long type name or a deep hierarchy path can hide the next block. +> +> ## What changed +> +> All twelve wrapping containers in the editor now go through +> `DxMessagingEditorTheme.ApplyContentSizedWrap`. It measures the children and gives the +> container that height. An editor that already sizes the container correctly is left alone. +> +> ## How we know +> +> A probe pins a wrapping row to one line's height, which is what 2021.3 does on its own. The +> unfixed probe leaves its children outside the row. The fixed probe does not. A source scan +> fails if any editor file turns wrapping on by hand again. + +95 words. Same change, and now a reviewer can check it. + +## Closing several issues in one pull request + +One `Why / What changed / How we know` block per issue. No shared preamble. + +> Closes #440, #336. +> +> ## Wrapped rows draw outside their box (#440) +> +> **Why.** On Unity 2021.3 a wrapping row keeps one line's height... +> +> **What changed.** ... +> +> **How we know.** ... +> +> ## The MSVC gate passes a compiler that cannot run (#336) +> +> **Why.** ... + +Use `###`-free bold labels when the blocks are short. The reader scans for the issue heading, +then reads three sentences. + +## When a fourth section is right + +Add one only when the reader must act on it, or must not be surprised by it: + +- **What is still open.** Name the part the change does not fix, and the issue that tracks it. +- **Risk.** Name the case you could not test, and why. + +Do not add a fourth section for what you considered and dropped. That is a session diary. + +## Evidence: what goes in the body + +Keep the verdict. Move the working. + +| Evidence | Where it goes | +| ------------------------------------------ | ------------------------------- | +| "793 tests pass, 0 fail" | Body | +| The full test list | Nowhere | +| "EditMode step drops from 113.8s to 36.3s" | Body | +| The per-run table it came from | The issue | +| "Reproduced on run 31764457664" | Body, with the link | +| Console output, stack traces, raw logs | The issue, in a collapsed block | + +The rule behind the table: a number a reviewer would check belongs in the body. The data you +derived it from belongs where it can be re-derived. + +## Titles + +| Bad | Why | Good | +| ---------------------------------------------------------------------------------------- | --------------------------------------------- | ------------------------------------------- | +| `fix: layout` | Says nothing | `Keep wrapped editor rows inside their box` | +| `Refactor DxMessagingFlowGraphWindow flexWrap handling` | Names the mechanism, not the effect | `Keep wrapped editor rows inside their box` | +| `Fix #440` | The reader cannot see the issue from the list | `Keep wrapped editor rows inside their box` | +| `Fix the wrap bug, add MSVC launch probe, close 5 measurement issues, update agent docs` | Four clauses | `Keep wrapped editor rows inside their box` | + +The last one is the common case for a session that closes several issues. Lead with the change +a user would notice. The body lists the rest. diff --git a/.llm/skills/shell-and-powershell/SKILL.md b/.llm/skills/shell-and-powershell/SKILL.md index 04ec139a..07ae5af6 100644 --- a/.llm/skills/shell-and-powershell/SKILL.md +++ b/.llm/skills/shell-and-powershell/SKILL.md @@ -99,6 +99,10 @@ application for this OS platform`, or a case-mismatch file-not-found. `os.tmpdir()` on `C:`), so `rel.startsWith("..")` mislabels an outside path as inside. Use `isPathInsideDirectory` / `isPathOutsideDirectory` / `isOutsideRelative` from `scripts/lib/path-classifier.js`; if you truly cannot, pair with `path.isAbsolute(rel)`. +- PowerShell parses a hex literal with no type suffix as a SIGNED integer, so `0xC0000000` is + `-1073741824`, not `3221225472`. Comparing a Windows exit code against a bare NTSTATUS + literal therefore matches everything. Suffix it (`0xC0000000L`) or write the decimal. See + `Test-CompilerLaunchOutcome` in `scripts/unity/assert-msvc-toolchain.ps1`. - GNU tar reads an archive operand containing an unqualified colon as a remote spec, so `tar -f C:\Temp\package.tgz` fails. Set the subprocess `cwd` to the archive directory and pass `./`; use `buildLocalTarArchiveSpec()` from `scripts/validate-npm-meta.js` diff --git a/CHANGELOG.md b/CHANGELOG.md index 53e432e8..59383c5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 already did, and the route-kind chips give space back instead of pushing the search field past the edge ([#435](https://github.com/Ambiguous-Interactive/DxMessaging/issues/435)). +- Keep wrapping rows in the Flow Graph and Message Monitor inside their own box. On Unity 2021.3 a + wrapping row keeps the height of one line, so a long type name or a deep hierarchy path pushed + the extra lines on top of the block beneath. Every wrapping container in the editor now measures + its lines and takes that height. Monitor lane pills and active-filter tokens also wrap in a row + the window owns rather than in the scroll view's own content container, which Unity sized to the + viewport and clipped, so tokens past the first lines can now be scrolled to + ([#440](https://github.com/Ambiguous-Interactive/DxMessaging/issues/440)). + ## [3.3.0] ### Added diff --git a/Editor/DxMessagingEditorTheme.cs b/Editor/DxMessagingEditorTheme.cs index 14b5fd20..bd8df672 100644 --- a/Editor/DxMessagingEditorTheme.cs +++ b/Editor/DxMessagingEditorTheme.cs @@ -2,6 +2,7 @@ namespace DxMessaging.Editor { using System; + using System.Collections.Generic; using UnityEditor; using UnityEngine; using UnityEngine.UIElements; @@ -413,6 +414,189 @@ internal static float ApplyResizedHeight( return clamped; } + /// + /// Turns wrapping on for a container and keeps that container at least as tall as the + /// lines its children wrap onto. + /// + /// Unity 2021.3 does not grow a wrapping container to fit the extra lines. The container + /// keeps its single-line height and the wrapped lines draw outside it, on top of whatever + /// the window draws beneath. Newer editors size the container correctly, so the defect is + /// invisible on 6000.4. Issue #435 hit it in the Message Monitor toolbar and issue #440 + /// lists the Flow Graph containers with the same shape. + /// + /// Measuring the children and applying the result as `min-height` supplies the height + /// Unity 2021.3 does not derive. The measurement is only applied when the container is + /// actually too short, so an editor that already sizes the container writes no inline + /// style at all. `align-content: flex-start` packs the lines at the top, so a container + /// that just grew cannot stretch its own lines and ask to grow again. + /// + /// The children are watched as well as the container. Unity reports a geometry change to + /// an element only when that element's own box changes, and it does not report a child's + /// change to the parent in either propagation phase. A container already held at a height + /// therefore never hears that its text finished measuring and needs more room, which is + /// how a details header ended up ten pixels short on Unity 6000.3. + /// + internal static void ApplyContentSizedWrap(VisualElement container) + { + if (container == null) + { + return; + } + + container.style.flexWrap = Wrap.Wrap; + container.style.alignContent = Align.FlexStart; + WrapHeightFit fit = new(container); + container.RegisterCallback( + static (_, state) => state.Fit(), + fit + ); + container.RegisterCallback( + static (_, state) => state.Release(), + fit + ); + } + + /// + /// The height a wrapping container needs for every line its children occupy, measured in + /// the container's own coordinate space so a panned or zoomed ancestor cannot skew it. + /// + /// Returns 0 when nothing is measurable yet, which includes a child laid out with an + /// unbounded size. Unity reports that as `Length`'s maximum, 8388608, and treating it as a + /// real height would ask for a box eight million pixels tall. + /// + internal static float MeasureWrappedContentHeight(VisualElement container) + { + if (container == null) + { + return 0f; + } + + float contentBottom = 0f; + bool measured = false; + foreach (VisualElement child in container.Children()) + { + if (child.resolvedStyle.display == DisplayStyle.None) + { + continue; + } + + // An absolutely positioned child is out of flow, so it creates no wrapped line + // and must not drag the container's height with it. + if (child.resolvedStyle.position == Position.Absolute) + { + continue; + } + + Rect childLayout = child.layout; + if (float.IsNaN(childLayout.yMax)) + { + continue; + } + + if (childLayout.yMax >= UnboundedLayoutSize) + { + return 0f; + } + + contentBottom = Mathf.Max( + contentBottom, + childLayout.yMax + child.resolvedStyle.marginBottom + ); + measured = true; + } + + if (!measured) + { + return 0f; + } + + IResolvedStyle containerStyle = container.resolvedStyle; + return contentBottom + containerStyle.paddingBottom + containerStyle.borderBottomWidth; + } + + /// + /// Unity's largest representable length (`Length`'s own maximum). A layout that reaches it + /// was measured with no bound rather than measured, so it is not a height to apply. + /// + private const float UnboundedLayoutSize = 8388608f; + + /// + /// Holds the height one container was given, so a container that already fits is never + /// written to and a container that was grown is only written again when it needs more. + /// + /// The height is only ever raised while the container is in a panel. Leaving the panel + /// releases it, which is what a reused or rebuilt container needs. + /// + private sealed class WrapHeightFit + { + private const float Tolerance = 0.5f; + + private readonly VisualElement _container; + private readonly HashSet _watchedChildren = new(); + + private bool _applied; + private float _appliedHeight; + + internal WrapHeightFit(VisualElement container) + { + _container = container; + } + + internal void Fit() + { + WatchChildren(); + + float required = MeasureWrappedContentHeight(_container); + if (required <= 0f || required >= UnboundedLayoutSize) + { + return; + } + + float current = _applied ? _appliedHeight : _container.resolvedStyle.height; + if (required <= current + Tolerance) + { + return; + } + + _applied = true; + _appliedHeight = required; + _container.style.minHeight = required; + } + + internal void Release() + { + _watchedChildren.Clear(); + if (!_applied) + { + return; + } + + _applied = false; + _appliedHeight = 0f; + _container.style.minHeight = StyleKeyword.Null; + } + + /// + /// A child that resizes in place changes what the container needs, and Unity does not + /// report a child's geometry change to its parent. Each child is watched directly. + /// + private void WatchChildren() + { + foreach (VisualElement child in _container.Children()) + { + if (!_watchedChildren.Add(child)) + { + continue; + } + + child.RegisterCallback( + static (_, state) => state.Fit(), + this + ); + } + } + } + private static StyleSheet LoadStyleSheet(string path) { return AssetDatabase.LoadAssetAtPath(path); diff --git a/Editor/Windows/DxMessagingFlowGraphWindow.cs b/Editor/Windows/DxMessagingFlowGraphWindow.cs index c7590873..555efadb 100644 --- a/Editor/Windows/DxMessagingFlowGraphWindow.cs +++ b/Editor/Windows/DxMessagingFlowGraphWindow.cs @@ -2463,7 +2463,7 @@ FlowGraphCanvasState canvasState VisualElement legend = new() { name = GraphLegendName }; legend.style.flexDirection = FlexDirection.Row; - legend.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(legend); legend.style.alignItems = Align.Center; legend.style.marginBottom = 6; @@ -6243,7 +6243,7 @@ Action onSelectionChanged VisualElement header = new(); header.AddToClassList(DxMessagingEditorTheme.DetailHeadClassName); - header.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(header); VisualElement heading = new(); heading.style.flexGrow = 1; heading.style.flexShrink = 1; @@ -6757,7 +6757,7 @@ Action onSelectionChanged VisualElement summary = new(); summary.style.flexDirection = FlexDirection.Row; - summary.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(summary); summary.style.alignItems = Align.Center; VisualElement receiver = CreateRelationshipIdentity("RECEIVER", string.Empty); receiver.style.flexGrow = 1; @@ -7109,7 +7109,7 @@ IEnumerable tracePaths row.Add(keyLabel); VisualElement values = new(); values.style.flexDirection = FlexDirection.Row; - values.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(values); values.style.flexGrow = 1; Label traceIdentity = new( widestTrace.PathCount <= 0 ? "none" : $"Trace #{widestTrace.TraceId}" @@ -7249,7 +7249,7 @@ private static VisualElement CreateRoutePathSection(FlowGraphEdge edge) VisualElement section = CreateDetailsSection("ROUTE"); VisualElement flow = new(); flow.style.flexDirection = FlexDirection.Row; - flow.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(flow); flow.style.alignItems = Align.Center; string kind = DxMessagingEditorPalette.NormalizeRouteKind(edge.RegistrationTypeName); string context = CreateReadableRouteContext(edge); @@ -7337,7 +7337,7 @@ params GraphNodeMetric[] metrics VisualElement section = CreateDetailsSection(title); VisualElement grid = new(); grid.style.flexDirection = FlexDirection.Row; - grid.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(grid); foreach (GraphNodeMetric metric in metrics) { VisualElement tile = new(); @@ -7498,7 +7498,7 @@ out string descriptor }; trail.AddToClassList(DetailsHierarchyTrailClassName); trail.style.flexDirection = FlexDirection.Row; - trail.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(trail); trail.style.alignItems = Align.Center; trail.style.flexGrow = 1; trail.style.flexShrink = 1; @@ -7689,7 +7689,7 @@ private static VisualElement CreateDetailsRelationship( VisualElement header = new(); header.style.flexDirection = FlexDirection.Row; - header.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(header); header.style.alignItems = Align.Center; Label relationshipLabel = new(label); relationshipLabel.AddToClassList(DxMessagingEditorTheme.CardLabelClassName); @@ -7703,7 +7703,7 @@ private static VisualElement CreateDetailsRelationship( VisualElement flow = new(); flow.style.flexDirection = FlexDirection.Row; - flow.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(flow); flow.style.alignItems = Align.Center; flow.style.marginTop = 6; flow.Add( @@ -7786,7 +7786,7 @@ string activityLabel { VisualElement activity = new(); activity.style.flexDirection = FlexDirection.Row; - activity.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(activity); activity.style.marginTop = 5; Label activityCount = new($"{deliveryCount} {activityLabel}"); activityCount.AddToClassList(DxMessagingEditorTheme.DetailFrameClassName); diff --git a/Editor/Windows/DxMessagingMessageMonitorWindow.cs b/Editor/Windows/DxMessagingMessageMonitorWindow.cs index d3e39c51..bd826d45 100644 --- a/Editor/Windows/DxMessagingMessageMonitorWindow.cs +++ b/Editor/Windows/DxMessagingMessageMonitorWindow.cs @@ -30,6 +30,8 @@ public sealed class DxMessagingMessageMonitorWindow : EditorWindow "dxmessaging-monitor-active-filter-label"; internal const string ActiveFilterTokenScrollViewName = "dxmessaging-monitor-active-filter-token-scroll"; + internal const string ActiveFilterTokenWrapRowName = + "dxmessaging-monitor-active-filter-token-row"; internal const string ActiveFilterTokenClassName = "dxmessaging-monitor-active-filter-token"; internal const string ActiveFilterClearButtonName = @@ -1369,6 +1371,24 @@ IReadOnlyList filteredEntries return breakdown; } + /// + /// A wrapping row for content inside a . + /// + /// The row belongs to this window rather than being the scroll view's own content + /// container. Unity sizes that container to the viewport, so a wrapping content container + /// keeps only the first screenful of lines: the rest are clipped and the scroller reports + /// nothing to scroll to. An owned row is sized by its own lines, which the scroll view + /// then scrolls (GitHub #440). + /// + private static VisualElement CreateScrollableWrapRow(string name) + { + VisualElement row = new() { name = name }; + row.style.flexDirection = FlexDirection.Row; + row.style.flexShrink = 0; + DxMessagingEditorTheme.ApplyContentSizedWrap(row); + return row; + } + private static VisualElement CreateLanePanel( string panelName, string summaryLabelName, @@ -1406,13 +1426,16 @@ IEnumerable pills laneRows.style.flexShrink = 1; laneRows.style.minHeight = 0; laneRows.style.marginTop = 4; - laneRows.contentContainer.style.flexDirection = FlexDirection.Row; - laneRows.contentContainer.style.flexWrap = Wrap.Wrap; + // Derived from the scroll view's own name: this panel is built twice, once for + // message-type lanes and once for context lanes, so a shared constant would put the + // same element name in the tree twice. + VisualElement laneRowsContent = CreateScrollableWrapRow(scrollViewName + "-row"); + laneRows.Add(laneRowsContent); lanesRoot.Add(laneRows); foreach (VisualElement pill in pills) { - laneRows.Add(pill); + laneRowsContent.Add(pill); } return lanesRoot; @@ -1897,8 +1920,8 @@ out string[] displayTokens tokenScroll.style.flexGrow = 1; tokenScroll.style.flexShrink = 1; tokenScroll.style.maxHeight = 72; - tokenScroll.contentContainer.style.flexDirection = FlexDirection.Row; - tokenScroll.contentContainer.style.flexWrap = Wrap.Wrap; + VisualElement tokenRow = CreateScrollableWrapRow(ActiveFilterTokenWrapRowName); + tokenScroll.Add(tokenRow); summary.Add(tokenScroll); foreach (string token in displayTokens) @@ -1917,7 +1940,7 @@ out string[] displayTokens DxMessagingEditorPalette.Border ); tokenLabel.style.whiteSpace = WhiteSpace.Normal; - tokenScroll.Add(tokenLabel); + tokenRow.Add(tokenLabel); } } diff --git a/README.md b/README.md index 89b40515..2d4563c2 100644 --- a/README.md +++ b/README.md @@ -985,7 +985,7 @@ DxMessaging publishes machine-readable context for AI agents through [llms.txt]( - **[llms.txt](llms.txt)** -- Complete project overview, API reference, and context in a single file - **[Repository Guidelines](.llm/context.md)** -- Coding standards and development workflows -- **[AI Agent Skills](.llm/skills/)** -- 31+ specialized skills covering documentation, testing, GitHub Actions, and more +- **[AI Agent Skills](.llm/skills/)** -- 32+ specialized skills covering documentation, testing, GitHub Actions, and more The `llms.txt` file is automatically updated via CI/CD to stay current with project changes. It includes: diff --git a/Tests/Editor/DxMessagingEditorThemeTests.cs b/Tests/Editor/DxMessagingEditorThemeTests.cs index 6ab61425..eee82030 100644 --- a/Tests/Editor/DxMessagingEditorThemeTests.cs +++ b/Tests/Editor/DxMessagingEditorThemeTests.cs @@ -25,6 +25,36 @@ public sealed class DxMessagingEditorThemeTests "white-space: nowrap;", }; + private const float ProbeWindowWidth = 240f; + private const float ProbeWindowHeight = 240f; + private const float ProbeChildWidth = 100f; + private const float ProbeChildHeight = 20f; + private const int ProbeChildCount = 4; + + /// + /// How many probe children fit on one line at . + /// + private const int SingleLineChildCount = 2; + + /// + /// Unity's largest representable length, which an element laid out with no bound reports. + /// + private const float UnboundedLayoutSize = 8388608f; + + /// + /// One line of children, which is exactly what Unity 2021.3 leaves a wrapping container at. + /// + private const float PinnedHeight = ProbeChildHeight; + + private readonly List _createdWindows = new(); + + [TearDown] + public void TearDown() + { + EditorWindowTestUtility.SuppressHeadlessWindowRenderErrors(); + EditorWindowTestUtility.CloseTrackedWindows(_createdWindows); + } + [Test] public void ThemeAssetsLoadFromPackagePaths() { @@ -670,6 +700,309 @@ public void ApplyCompleteBorderSupportsUniformCustomWidth() AssertColor(element.style.borderLeftColor.value, DxMessagingEditorPalette.AmberSoft); } + /// + /// Issue #440: on Unity 2021.3 a wrapping container keeps its single-line height, so the + /// lines its children wrap onto draw outside it and over whatever the window paints + /// beneath. Pinning the height reproduces that refusal to grow on every editor, which is + /// what makes this red-green here as well as on the 2021.3 leg. + /// + [Test] + public void AWrappingContainerTooShortForItsLinesGrowsUntilEveryChildIsInsideIt() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement control = AddWrapProbe(window, PinnedHeight, contentSized: false); + VisualElement fixedProbe = AddWrapProbe(window, PinnedHeight, contentSized: true); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(control), + Is.GreaterThan( + control.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The control probe must reproduce the defect, otherwise the fixed probe below " + + "proves nothing: a wrapping container that will not grow leaves its " + + "wrapped lines outside its own box." + ); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(fixedProbe), + Is.LessThanOrEqualTo( + fixedProbe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "A container built through ApplyContentSizedWrap must end up tall enough for " + + "every line its children wrap onto." + ); + } + + /// + /// An editor that already sizes a wrapping container must be left alone: no inline + /// `min-height` is written, so the helper cannot lower a height the stylesheet declared. + /// + [Test] + public void AWrappingContainerThatAlreadyFitsIsLeftWithoutAnInlineHeight() + { + EditorWindow window = CreateWrapProbeWindow(); + + // Two children fit on one line, so no editor has to derive a wrapped height and this + // reads the same on Unity 2021.3 as it does on 6000.x. + VisualElement probe = AddWrapProbe( + window, + pinnedHeight: 0f, + contentSized: true, + childCount: SingleLineChildCount + ); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.LessThanOrEqualTo( + probe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The probe must already fit, otherwise this asserts the wrong case." + ); + // Unity reports an unset inline style as `Null` on 6000.4 and as `Undefined` on + // 2021.3 and 2022.3. Both mean the same thing: no inline value was written. + Assert.That( + probe.style.minHeight.keyword, + Is.EqualTo(StyleKeyword.Null).Or.EqualTo(StyleKeyword.Undefined), + "A container that already fits its lines must keep the height its stylesheet " + + "gives it, so the helper must not write an inline min-height." + ); + } + + /// + /// A container that leaves the panel is about to be rebuilt or reused, so it must forget + /// the height its last content needed. Otherwise a details pane that once held a deep + /// hierarchy trail keeps that gap for every selection after it. + /// + [Test] + public void AGrownWrappingContainerForgetsItsHeightWhenItLeavesThePanel() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement probe = AddWrapProbe(window, PinnedHeight, contentSized: true); + EditorWindowTestUtility.SettleLayout(window); + Assert.That( + probe.resolvedStyle.height, + Is.GreaterThan(PinnedHeight + EditorWindowTestUtility.LayoutTolerance), + "The probe must have grown first, otherwise there is no height to forget." + ); + + probe.RemoveFromHierarchy(); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + probe.style.minHeight.keyword, + Is.EqualTo(StyleKeyword.Null), + "Leaving the panel must release the height, so the container measures its next " + + "content from its own stylesheet height." + ); + + while (probe.childCount > SingleLineChildCount) + { + probe.RemoveAt(probe.childCount - 1); + } + window.rootVisualElement.Add(probe); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + probe.resolvedStyle.height, + Is.EqualTo(PinnedHeight).Within(EditorWindowTestUtility.LayoutTolerance), + "Two children need one line, so the re-added container must be one line tall." + ); + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.LessThanOrEqualTo( + probe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The re-measured container must still contain its children." + ); + } + + /// + /// Unity reports a geometry change to an element only when that element's own box changes, + /// and never reports a child's change to the parent. So a container already held at a + /// height hears nothing when its content grows in place, which is how a Flow Graph details + /// header ended up ten pixels short on Unity 6000.3 with the fix applied. + /// + [Test] + public void AWrappingContainerGrowsAgainWhenAChildGrowsInPlace() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement probe = AddWrapProbe(window, PinnedHeight, contentSized: true); + EditorWindowTestUtility.SettleLayout(window); + float firstHeight = probe.resolvedStyle.height; + + probe[probe.childCount - 1].style.height = ProbeChildHeight * 3f; + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + probe.resolvedStyle.height, + Is.GreaterThan(firstHeight + EditorWindowTestUtility.LayoutTolerance), + "The container must grow again when a child grows, not stay at the height its " + + "first content needed." + ); + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.LessThanOrEqualTo( + probe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The grown child must end up inside the container." + ); + } + + /// + /// A child laid out with no bound reports Unity's largest length instead of a measurement. + /// Treating it as a height asks for a box eight million pixels tall, which is what the + /// Unity 2021.3, 2022.3 and 6000.3 legs reported before this guard existed. + /// + [Test] + public void AnUnboundedChildIsNotMistakenForAHeightToApply() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement probe = AddWrapProbe(window, pinnedHeight: 0f, contentSized: true); + probe[0].style.height = UnboundedLayoutSize; + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.EqualTo(0f), + "An unbounded child is not a measurement, so there is nothing to report." + ); + Assert.That( + probe.style.minHeight.keyword, + Is.EqualTo(StyleKeyword.Null).Or.EqualTo(StyleKeyword.Undefined), + "The helper must not write an eight-million-pixel min-height." + ); + } + + /// + /// Drift guard for the whole class of defect in issue #440: turning wrapping on by hand, + /// in a source file or in a stylesheet, re-creates it, because nothing then supplies the + /// height Unity 2021.3 does not derive. Everything shipped must go through the helper. + /// + [Test] + public void EveryEditorWrapContainerGoesThroughTheContentSizedWrapHelper() + { + const string bannedDeclaration = "style.flex" + "Wrap"; + string editorRoot = DxMessagingEditorTheme.PackageRoot + "/Editor"; + string allowedRelativePath = "Editor/DxMessagingEditorTheme.cs"; + + string[] sourcePaths = System.IO.Directory.GetFiles( + editorRoot, + "*.cs", + System.IO.SearchOption.AllDirectories + ); + Assert.That( + sourcePaths, + Is.Not.Empty, + $"Expected editor sources under '{editorRoot}'." + ); + + List offenders = new(); + foreach (string sourcePath in sourcePaths) + { + string normalized = sourcePath.Replace('\\', '/'); + if (normalized.EndsWith(allowedRelativePath, StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + string[] lines = System.IO.File.ReadAllLines(sourcePath); + for (int index = 0; index < lines.Length; index++) + { + if (lines[index].Contains(bannedDeclaration)) + { + offenders.Add($"{normalized}:{index + 1}"); + } + } + } + + // A stylesheet can turn wrapping on just as well as a source file can, and + // nothing would then supply the height either. + foreach ( + string ussPath in new[] + { + DxMessagingEditorTheme.TokensUssPath, + DxMessagingEditorTheme.ThemeUssPath, + } + ) + { + string[] ussLines = System.IO.File.ReadAllLines(ussPath); + for (int index = 0; index < ussLines.Length; index++) + { + if (ussLines[index].Contains("flex-" + "wrap:")) + { + offenders.Add($"{ussPath}:{index + 1}"); + } + } + } + + Assert.That( + offenders, + Is.Empty, + "These editor sources or stylesheets turn wrapping on directly instead of calling " + + "DxMessagingEditorTheme.ApplyContentSizedWrap, so on Unity 2021.3 their " + + "wrapped lines draw outside the container (GitHub #440):\n" + + string.Join("\n", offenders) + ); + } + + private EditorWindow CreateWrapProbeWindow() + { + EditorWindow window = EditorWindowTestUtility.CreateWindow(); + _createdWindows.Add(window); + window.position = new Rect(0f, 0f, ProbeWindowWidth, ProbeWindowHeight); + EditorWindowTestUtility.ShowWindow(window); + return window; + } + + /// + /// A row of equal children too wide to fit on one line. A non-zero + /// holds the container at one line's worth of height, + /// which is what Unity 2021.3 does to a wrapping container on its own. + /// + private static VisualElement AddWrapProbe( + EditorWindow window, + float pinnedHeight, + bool contentSized, + int childCount = ProbeChildCount + ) + { + VisualElement probe = new(); + probe.style.flexDirection = FlexDirection.Row; + probe.style.width = ProbeWindowWidth; + probe.style.flexShrink = 0f; + if (contentSized) + { + DxMessagingEditorTheme.ApplyContentSizedWrap(probe); + } + else + { + probe.style.flexWrap = Wrap.Wrap; + } + + if (pinnedHeight > 0f) + { + probe.style.height = pinnedHeight; + } + + for (int index = 0; index < childCount; index++) + { + VisualElement child = new(); + child.style.width = ProbeChildWidth; + child.style.height = ProbeChildHeight; + child.style.flexShrink = 0f; + probe.Add(child); + } + + window.rootVisualElement.Add(probe); + return probe; + } + private static void AssertIconLoads(string fileName, int expectedSize) { Texture2D icon = DxMessagingEditorTheme.LoadIcon(fileName); diff --git a/Tests/Editor/DxMessagingFlowGraphWindowTests.cs b/Tests/Editor/DxMessagingFlowGraphWindowTests.cs index 4f648ed4..cd0e499a 100644 --- a/Tests/Editor/DxMessagingFlowGraphWindowTests.cs +++ b/Tests/Editor/DxMessagingFlowGraphWindowTests.cs @@ -105,6 +105,17 @@ public sealed class DxMessagingFlowGraphWindowTests private const string TraceIdLaneDetailsLabelName = "dxmessaging-flow-graph-trace-id-lane-details"; + /// + /// The smallest the Flow Graph window can be made, from its own `minSize`. Containers wrap + /// here first, so this is where issue #440 shows up. + /// + private const float FlowGraphMinimumWidth = 520f; + private const float FlowGraphMinimumHeight = 360f; + + private const string EdgeSelection = "route"; + private const string ComponentSelection = "component"; + private const string MessageSelection = "message"; + private bool _stackTracesBeforeTest; [SetUp] @@ -11472,6 +11483,126 @@ private static FlowGraphSnapshot CreateStableContextRouteSnapshot(string context ); } + /// + /// Issue #440: ten Flow Graph containers wrap. Unity 2021.3 does not grow a wrapping + /// container to fit the lines its children wrap onto, so those lines draw outside the + /// container and over the block beneath it. What makes them wrap is long type names and a + /// deep hierarchy trail - short in a fixture, long in a real project - so this renders + /// both, at the smallest window a reader can make. + /// + [TestCase(EdgeSelection)] + [TestCase(ComponentSelection)] + [TestCase(MessageSelection)] + public void EveryWrappingContainerStaysInsideItsOwnBoxAtTheWindowsMinimumSize( + string selection + ) + { + FlowGraphSnapshot snapshot = CreateLongContentSnapshot(); + string selectedItemKey = selection switch + { + EdgeSelection => DxMessagingFlowGraphWindow.CreateEdgeSelectionKey( + snapshot.Edges[0] + ), + ComponentSelection => DxMessagingFlowGraphWindow.CreateComponentSelectionKey( + snapshot.ComponentNodes[0] + ), + MessageSelection => DxMessagingFlowGraphWindow.CreateMessageSelectionKey( + snapshot.MessageNodes[0] + ), + _ => throw new ArgumentOutOfRangeException(nameof(selection), selection, null), + }; + + EditorWindow window = CreateTrackedEditorWindow(); + window.position = new Rect(0f, 0f, FlowGraphMinimumWidth, FlowGraphMinimumHeight); + EditorWindowTestUtility.ShowWindow(window); + + DxMessagingFlowGraphWindow.BuildGraphUi( + window.rootVisualElement, + snapshot, + new FlowGraphViewState(selectedItemKey: selectedItemKey) + ); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + window.rootVisualElement.Q( + DxMessagingFlowGraphWindow.DetailsPaneName + ), + Is.Not.Null, + "The details pane carries most of the wrapping containers, so it has to render." + ); + EditorWindowTestUtility.AssertWrappingContainersContainTheirChildren( + window.rootVisualElement, + $"The Flow Graph at its minimum window size with a {selection} selected" + ); + } + + /// + /// A snapshot whose names are as long as a real project's: a deep hierarchy trail, a + /// namespaced message type, and traces, so the hierarchy trail, metric grid, route flow + /// and widest-trace rows all have enough content to wrap. + /// + private static FlowGraphSnapshot CreateLongContentSnapshot() + { + const string componentId = "component:damage-resolution"; + const string hierarchyPath = + "Root/Systems/Gameplay/Combat/Encounters/EliteWaveDirector/DamageResolutionListener"; + const string messageTypeName = + "Gameplay.Combat.Damage.DamageResolutionRequestedBroadcastMessage"; + + return new FlowGraphSnapshot( + new[] + { + new FlowGraphComponentNode( + componentId, + hierarchyPath, + "PlayerCombatDamageResolutionMessagingComponent", + activeInHierarchy: true, + listenerCount: 3, + registrationCount: 4, + callCount: 128, + localMessageCount: 6 + ), + }, + new[] + { + new FlowGraphMessageNode( + messageTypeName, + registrationCount: 4, + callCount: 128, + recentGlobalEmissionCount: 64, + recentLocalMessageCount: 6, + recentTracedDeliveryCount: 12 + ), + }, + new[] + { + new FlowGraphEdge( + messageTypeName, + componentId, + hierarchyPath, + "Broadcast", + registrationCount: 4, + callCount: 128, + recentTracedDeliveryCount: 12, + context: hierarchyPath + ), + }, + new[] + { + new FlowGraphTracePath( + messageTypeName, + hierarchyPath, + componentId, + hierarchyPath, + "Broadcast", + recentTracedDeliveryCount: 12, + traceIds: new long[] { 8801, 8802, 8803, 8804 } + ), + }, + Array.Empty() + ); + } + private static FlowGraphSnapshot CreateTwoEdgeSnapshot() { return new FlowGraphSnapshot( diff --git a/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs b/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs index 7223f628..509258fd 100644 --- a/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs +++ b/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs @@ -26,6 +26,9 @@ public sealed class DxMessagingMessageMonitorWindowTests "Packages/com.wallstop-studios.dxmessaging/Tests/Editor/Fixtures/EditModeSceneSafety.unity"; private readonly List _createdObjects = new(); private readonly List _createdAssetPaths = new(); + private const float MonitorMinimumWidth = 420f; + private const float MonitorMinimumHeight = 320f; + private readonly List _createdWindows = new(); private const string MessageTypeLanesName = "dxmessaging-monitor-message-type-lanes"; private const string MessageTypeLaneScrollViewName = @@ -3725,6 +3728,101 @@ public void CaptureSnapshotReadsDefaultMessageBusHistory() Assert.That(snapshot.Entries[1].MessageTypeName, Is.EqualTo(nameof(OlderMessage))); } + /// + /// Wrapping the scroll view's own content container clipped the token list to the + /// viewport and left the scroller with nothing to scroll, so every token past the first + /// few lines was unreachable. The tokens wrap in a row this window owns, which the scroll + /// view can then scroll (GitHub #440). + /// + [Test] + public void ActiveFilterTokensStayReachableWhenTheyWrapPastTheScrollViewHeight() + { + MessageMonitorSnapshot snapshot = new( + diagnosticsEnabled: true, + capacity: 8, + entries: new[] { CreateEntry(new OlderMessage(), null) } + ); + string longFilter = string.Join( + " ", + Enumerable.Range(0, 24).Select(index => $"type:Message{index:00}") + ); + EditorWindow window = CreateTrackedEditorWindow(); + window.position = new Rect(0f, 0f, MonitorMinimumWidth, MonitorMinimumHeight); + EditorWindowTestUtility.ShowWindow(window); + + DxMessagingMessageMonitorWindow.BuildMonitorUi( + window.rootVisualElement, + snapshot, + new MessageMonitorViewState(longFilter) + ); + EditorWindowTestUtility.SettleLayout(window); + + ScrollView tokenScroll = window + .rootVisualElement.Q(ActiveFilterSummaryName) + .Q(ActiveFilterTokenScrollViewName); + VisualElement tokenRow = tokenScroll.Q( + DxMessagingMessageMonitorWindow.ActiveFilterTokenWrapRowName + ); + + Assert.That(tokenRow, Is.Not.Null, "The tokens must wrap in a row the window owns."); + Assert.That( + tokenRow.resolvedStyle.height, + Is.GreaterThan(tokenScroll.contentViewport.resolvedStyle.height), + "The probe must overflow the viewport, otherwise there is nothing to scroll to." + ); + Assert.That( + tokenScroll.verticalScroller.highValue, + Is.GreaterThan(0f), + "Every token past the first visible line must be reachable by scrolling." + ); + } + + /// + /// Issue #440: the Monitor's lane rows and active-filter tokens are wrapping containers + /// too. Unity 2021.3 does not grow a wrapping container to fit the lines its children wrap + /// onto, so those lines fall outside their scroll view's content and cannot be scrolled to. + /// A long filter and several message kinds are what fills both at the minimum window size. + /// + [Test] + public void EveryWrappingContainerStaysInsideItsOwnBoxAtTheWindowsMinimumSize() + { + MessageMonitorSnapshot snapshot = new( + diagnosticsEnabled: true, + capacity: 8, + entries: new[] + { + CreateEntry(new OlderMessage(), null), + CreateEntry(new NewerMessage(), new InstanceId(123)), + } + ); + string longFilter = string.Join( + " ", + Enumerable.Range(0, 24).Select(index => $"type:Message{index:00}") + ); + EditorWindow window = CreateTrackedEditorWindow(); + window.position = new Rect(0f, 0f, MonitorMinimumWidth, MonitorMinimumHeight); + EditorWindowTestUtility.ShowWindow(window); + + DxMessagingMessageMonitorWindow.BuildMonitorUi( + window.rootVisualElement, + snapshot, + new MessageMonitorViewState(longFilter) + ); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + window + .rootVisualElement.Q(ActiveFilterSummaryName) + .Q(ActiveFilterTokenScrollViewName), + Is.Not.Null, + "The active-filter token scroll view is one of the wrapping containers this covers." + ); + EditorWindowTestUtility.AssertWrappingContainersContainTheirChildren( + window.rootVisualElement, + "The Message Monitor at its minimum window size" + ); + } + private static MessageMonitorEntry CreateEntry(IMessage message, InstanceId? context) { return MessageMonitorEntry.FromEmission(new MessageEmissionData(message, context)); diff --git a/Tests/Editor/EditorWindowTestUtility.cs b/Tests/Editor/EditorWindowTestUtility.cs index 448161a4..372add13 100644 --- a/Tests/Editor/EditorWindowTestUtility.cs +++ b/Tests/Editor/EditorWindowTestUtility.cs @@ -3,9 +3,13 @@ namespace DxMessaging.Tests.Editor { using System; using System.Collections.Generic; + using System.Text; + using DxMessaging.Editor; + using NUnit.Framework; using UnityEditor; using UnityEngine; using UnityEngine.TestTools; + using UnityEngine.UIElements; using Object = UnityEngine.Object; internal sealed class DxMessagingTestHostWindow : EditorWindow @@ -17,6 +21,23 @@ internal static class EditorWindowTestUtility { private static readonly List CreatedWindows = new(); + /// + /// Sub-pixel slack for layout comparisons: Yoga rounds to the panel's pixel grid. + /// + internal const float LayoutTolerance = 0.5f; + + /// + /// Unity's largest representable length. An element reporting it was laid out with no + /// bound rather than measured. + /// + private const float UnboundedLayoutSize = 8388608f; + + /// + /// How many layout passes will run before giving up on the + /// tree settling. Real windows settle in two or three. + /// + private const int MaxLayoutPasses = 8; + internal static DxMessagingTestHostWindow CreateWindow() { DxMessagingTestHostWindow window = @@ -213,6 +234,134 @@ internal static void IgnoreUnityInvalidGcHandleAsserts(Action action) } } + /// + /// Runs the panel's layout until it stops changing, so a test reads the geometry a reader + /// settles on rather than an intermediate frame. + /// + /// One pass is never enough. Realizing content changes what elements measure, text height + /// is only final once its width is, and a + /// container asks for its height + /// during a pass and receives it in the next one. Unity settles this over frames; a test + /// has to ask for the frames. + /// + internal static void SettleLayout(EditorWindow window) + { + if (window == null) + { + return; + } + + string previous = null; + for (int pass = 0; pass < MaxLayoutPasses; pass++) + { + EditorSurfaceCapture.InvokeInheritedPanelMethod( + window.rootVisualElement.panel, + "ValidateLayout", + Array.Empty() + ); + + string current = DescribeLayout(window.rootVisualElement); + if (current == previous) + { + return; + } + + previous = current; + } + } + + private static string DescribeLayout(VisualElement root) + { + StringBuilder description = new(); + foreach (VisualElement element in root.Query().ToList()) + { + Rect layout = element.layout; + description + .Append(layout.x) + .Append(',') + .Append(layout.y) + .Append(',') + .Append(layout.width) + .Append(',') + .Append(layout.height) + .Append(';'); + } + + return description.ToString(); + } + + /// + /// Asserts every wrapping container under is tall enough for the + /// lines its children wrap onto. + /// + /// Unity 2021.3 does not grow a wrapping container to fit those lines (issues #435 and + /// #440), so the extra lines draw outside the container, on top of whatever the window + /// paints beneath. This assertion is what reports that on the 2021.3 leg; on newer editors + /// it holds without the fix, because they size the container correctly. + /// + internal static void AssertWrappingContainersContainTheirChildren( + VisualElement root, + string context + ) + { + Assert.That(root, Is.Not.Null, $"{context} must render a root element."); + + List wrapping = new(); + foreach (VisualElement element in root.Query().ToList()) + { + if (element.resolvedStyle.flexWrap != Wrap.Wrap || element.childCount == 0) + { + continue; + } + + // An element laid out with no bound reports Unity's largest length rather than a + // measurement. A box eight million pixels tall is not painting over anything a + // reader can see, and asserting on it compares two sentinels. + if ( + float.IsNaN(element.resolvedStyle.height) + || element.resolvedStyle.height >= UnboundedLayoutSize + ) + { + continue; + } + + wrapping.Add(element); + } + + Assert.That( + wrapping, + Is.Not.Empty, + $"{context} renders no wrapping container, so this assertion would pass " + + "without checking anything. Point it at a surface that wraps." + ); + + foreach (VisualElement element in wrapping) + { + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(element), + Is.LessThanOrEqualTo(element.resolvedStyle.height + LayoutTolerance), + $"{context}: wrapping container '{DescribeElement(element)}' is shorter than " + + "the lines its children wrap onto, so those lines draw outside it and " + + "over whatever the window paints beneath." + ); + } + } + + private static string DescribeElement(VisualElement element) + { + if (!string.IsNullOrEmpty(element.name)) + { + return element.name; + } + + foreach (string className in element.GetClasses()) + { + return "." + className; + } + + return element.GetType().Name; + } + private static System.Type FindUnityEditorType(string fullName) { foreach ( diff --git a/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs b/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs index 6f0e0133..a894f83e 100644 --- a/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs +++ b/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs @@ -5,6 +5,7 @@ namespace DxMessaging.Tests.Runtime { using System; using System.Diagnostics; + using System.Globalization; using NUnit.Framework; using NUnit.Framework.Interfaces; @@ -165,9 +166,14 @@ public void EndSuiteTimer() // included because the hard budget is selected per version (the // 2021.x runner gets a wider ceiling); seeing both together makes // a near-budget run easy to triage. + // Invariant culture on purpose: CI lifts this line into the job summary + // (issue #410), so the decimal separator must not follow the runner's + // locale. `scripts/unity/run-ci-tests.ps1` parses exactly this shape. + CultureInfo invariant = CultureInfo.InvariantCulture; UnityEngine.Debug.Log( - $"DxMessaging suite wall clock: {elapsed.TotalSeconds:0.00}s " - + $"(soft budget {SoftBudget.TotalSeconds:0.0}s, hard budget {HardBudget.TotalSeconds:0.0}s " + $"DxMessaging suite wall clock: {elapsed.TotalSeconds.ToString("0.00", invariant)}s " + + $"(soft budget {SoftBudget.TotalSeconds.ToString("0.0", invariant)}s, " + + $"hard budget {HardBudget.TotalSeconds.ToString("0.0", invariant)}s " + $"for Unity {UnityEngine.Application.unityVersion})." ); diff --git a/llms.txt b/llms.txt index 7fa1a29f..a6234762 100644 --- a/llms.txt +++ b/llms.txt @@ -214,7 +214,7 @@ npm run check:spelling This repository includes comprehensive AI agent guidance in the `.llm/` directory: - **[.llm/context.md](https://github.com/Ambiguous-Interactive/DxMessaging/blob/master/.llm/context.md)** - Repository guidelines, coding standards, testing policies -- **[.llm/skills/](https://github.com/Ambiguous-Interactive/DxMessaging/tree/master/.llm/skills)** - 31+ specialized skills: +- **[.llm/skills/](https://github.com/Ambiguous-Interactive/DxMessaging/tree/master/.llm/skills)** - 32+ specialized skills: - **allocation-and-leak-coverage** - **api-design-patterns** - **benchmark-methodology** @@ -232,6 +232,7 @@ This repository includes comprehensive AI agent guidance in the `.llm/` director - **memory-reclamation** - **object-pooling** - **package-publishing** + - **pull-request-writing** - **serializable-dictionary** - **shell-and-powershell** - **singleton-patterns** @@ -326,5 +327,5 @@ Copyright (c) 2017-2026 Wallstop Studios --- -**Last Updated:** 2026-08-16 +**Last Updated:** 2026-08-22 **Generated by:** scripts/update-llms-txt.js using package.json v3.3.0 and .llm/skills metadata diff --git a/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js new file mode 100644 index 00000000..e70d965b --- /dev/null +++ b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js @@ -0,0 +1,172 @@ +"use strict"; + +// Issue #410: a step regression stayed green for two days because nothing in CI +// looks at how long a step takes. `SuiteWallClockBudgetTest` already logs the +// suite's elapsed time and its budgets; `Write-SuiteWallClockSummary` lifts that +// one line into the job summary and warns when it is over the soft budget. +// +// These tests dot-source the function out of run-ci-tests.ps1 and drive it with +// fixture logs, so every branch runs without Unity. + +const { test } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); +const { spawnSync } = require("node:child_process"); + +const RUN_CI_SCRIPT_PATH = path.join(__dirname, "..", "unity", "run-ci-tests.ps1"); + +function commandExists(command) { + // prettier-ignore + const result = spawnSync(command, ["-NoLogo", "-NoProfile", "-Command", "$PSVersionTable.PSVersion"], + { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); + return !result.error && result.status === 0; +} + +const HAS_PWSH = commandExists("pwsh"); + +// The function is defined inside a script that runs a whole Unity leg when +// executed, so the body is extracted by name rather than dot-sourced. +function extractFunction(source, name) { + const start = source.indexOf(`function ${name} {`); + assert.notEqual(start, -1, `${name} must exist in run-ci-tests.ps1`); + const end = source.indexOf("\n}\n", start); + assert.notEqual(end, -1, `${name} must be a complete function`); + return source.slice(start, end + 3); +} + +function runSummary({ logText, label = "editmode" }) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "dxm-wallclock-")); + try { + const logPath = path.join(root, "unity.log"); + fs.writeFileSync(logPath, logText, "utf8"); + const summaryPath = path.join(root, "summary.md"); + const source = fs.readFileSync(RUN_CI_SCRIPT_PATH, "utf8"); + const scriptPath = path.join(root, "probe.ps1"); + fs.writeFileSync( + scriptPath, + [ + "param([string]$Label)", + "Set-StrictMode -Version Latest", + "$ErrorActionPreference = 'Stop'", + extractFunction(source, "Write-SuiteWallClockSummary"), + `Write-SuiteWallClockSummary -LogPath '${logPath}' -Label $Label` + ].join("\n"), + "utf8" + ); + + // The workflow runs this script once per test mode, so each leg is its own + // process. Driving it the same way is what proves the header is written once + // per job rather than once per leg. + const legs = [label, `${label}-second`]; + const runs = legs.map((leg) => + spawnSync("pwsh", ["-NoLogo", "-NoProfile", "-File", scriptPath, "-Label", leg], { + encoding: "utf8", + env: { ...process.env, GITHUB_STEP_SUMMARY: summaryPath } + }) + ); + const summary = fs.existsSync(summaryPath) ? fs.readFileSync(summaryPath, "utf8") : ""; + return { + stdout: runs.map((r) => r.stdout ?? "").join(""), + stderr: runs.map((r) => r.stderr ?? "").join(""), + status: runs.find((r) => r.status !== 0)?.status ?? 0, + summary + }; + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +const UNDER_BUDGET = + "DxMessaging suite wall clock: 36.30s (soft budget 60.0s, hard budget 180.0s for Unity 6000.4.6f1).\n"; +const OVER_BUDGET = + "DxMessaging suite wall clock: 113.80s (soft budget 60.0s, hard budget 180.0s for Unity 6000.4.6f1).\n"; + +test("a run under its soft budget is reported without a warning", { skip: !HAS_PWSH }, () => { + const run = runSummary({ logText: `noise\n${UNDER_BUDGET}more noise\n` }); + + assert.equal(run.status, 0, run.stderr); + assert.match(run.summary, /\| editmode \| 36\.30s \| 60\.0s \| 180\.0s \|/); + assert.doesNotMatch(run.stdout, /::warning::/); +}); + +test("a run over its soft budget warns and names both numbers", { skip: !HAS_PWSH }, () => { + const run = runSummary({ logText: OVER_BUDGET }); + + assert.equal(run.status, 0, run.stderr); + assert.match( + run.stdout, + /::warning::editmode suite wall clock 113\.80s is over its 60\.0s soft budget/ + ); + assert.match(run.summary, /\| editmode \| 113\.80s \| 60\.0s \| 180\.0s \|/); +}); + +test( + "the summary table header is written once per job, not once per leg", + { skip: !HAS_PWSH }, + () => { + const run = runSummary({ logText: UNDER_BUDGET }); + + const headers = run.summary.match(/### Suite wall clock/g) ?? []; + assert.equal(headers.length, 1, run.summary); + assert.match(run.summary, /\| editmode-second \|/); + } +); + +test("a log with no wall-clock line reports nothing", { skip: !HAS_PWSH }, () => { + const run = runSummary({ logText: "Unity started\nUnity finished\n" }); + + assert.equal(run.status, 0, run.stderr); + assert.equal(run.summary, ""); + assert.doesNotMatch(run.stdout, /::warning::/); +}); + +test("a missing log file reports nothing instead of failing the leg", { skip: !HAS_PWSH }, () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "dxm-wallclock-")); + try { + const source = fs.readFileSync(RUN_CI_SCRIPT_PATH, "utf8"); + const scriptPath = path.join(root, "probe.ps1"); + fs.writeFileSync( + scriptPath, + [ + "Set-StrictMode -Version Latest", + "$ErrorActionPreference = 'Stop'", + extractFunction(source, "Write-SuiteWallClockSummary"), + `Write-SuiteWallClockSummary -LogPath '${path.join(root, "absent.log")}' -Label 'playmode'` + ].join("\n"), + "utf8" + ); + const result = spawnSync("pwsh", ["-NoLogo", "-NoProfile", "-File", scriptPath], { + encoding: "utf8" + }); + assert.equal(result.status, 0, result.stderr); + assert.doesNotMatch(result.stdout ?? "", /::warning::/); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +}); + +test("the wall-clock line the harness parses is the one the suite emits", () => { + // The producing side is C#, the consuming side PowerShell. Neither compiles + // the other, so the shared shape is pinned here. + const budgetTest = fs.readFileSync( + path.join(__dirname, "..", "..", "Tests", "Runtime", "Core", "SuiteWallClockBudgetTest.cs"), + "utf8" + ); + assert.match( + budgetTest, + /DxMessaging suite wall clock: \{elapsed\.TotalSeconds\.ToString\("0\.00", invariant\)\}s/ + ); + assert.match( + budgetTest, + /soft budget \{SoftBudget\.TotalSeconds\.ToString\("0\.0", invariant\)\}s/ + ); + assert.match( + budgetTest, + /hard budget \{HardBudget\.TotalSeconds\.ToString\("0\.0", invariant\)\}s/ + ); + + const harness = fs.readFileSync(RUN_CI_SCRIPT_PATH, "utf8"); + assert.match(harness, /DxMessaging suite wall clock:\\s\*\(\[0-9\.\]\+\)s/); +}); diff --git a/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js.meta b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js.meta new file mode 100644 index 00000000..29d33c17 --- /dev/null +++ b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: 353475020b1cb4d8693a71e665d6ebda +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/scripts/__tests__/test-msvc-toolchain.ps1 b/scripts/__tests__/test-msvc-toolchain.ps1 index 4efc8c0f..fc04513f 100644 --- a/scripts/__tests__/test-msvc-toolchain.ps1 +++ b/scripts/__tests__/test-msvc-toolchain.ps1 @@ -31,12 +31,28 @@ function Assert-Verdict { [string]$Name, [string]$InstallRoot, [string[]]$ExecutablePaths = @(), + [string[]]$LaunchablePaths, [string]$ExpectedReason, - [bool]$ExpectedOk + [bool]$ExpectedOk, + [bool]$ExpectedAdvisory = $false ) $script:Count++ $probe = { param($path) $ExecutablePaths -contains $path }.GetNewClosure() - $result = Test-MsvcToolchain -InstallRoot $InstallRoot -ProbeExecutable $probe -RunnerName 'TEST-RUNNER' + $arguments = @{ + InstallRoot = $InstallRoot + ProbeExecutable = $probe + RunnerName = 'TEST-RUNNER' + } + if ($PSBoundParameters.ContainsKey('LaunchablePaths')) { + $arguments['ProbeLaunch'] = { param($path) $LaunchablePaths -contains $path }.GetNewClosure() + } + $result = Test-MsvcToolchain @arguments + if ($result.Advisory -ne $ExpectedAdvisory) { + Write-Output ("FAIL $Name`n" + + " expected Advisory=$ExpectedAdvisory, got $($result.Advisory)") + $script:Failures++ + return + } if ($result.Ok -ne $ExpectedOk -or $result.Reason -ne $ExpectedReason) { Write-Output ("FAIL $Name`n" + " expected Ok=$ExpectedOk Reason=$ExpectedReason`n" + @@ -104,6 +120,69 @@ try { } elseif ($VerboseOutput) { Write-Output 'ok newest toolset is chosen by version, not by name' } + # LAUNCH PROBE (#336). Presence alone reports a `cl.exe` that cannot start as + # healthy, and the leg then dies after taking a licence seat and the build + # lock. The probe is injected exactly like the presence probe, so both the + # healthy and the corrupt case run here rather than needing a Windows host. + Assert-Verdict -Name 'a compiler that starts passes the launch probe' ` + -InstallRoot $root -ExecutablePaths @($missing) -LaunchablePaths @($missing) ` + -ExpectedOk $true -ExpectedReason 'ok' + + # A present compiler that will not start is its own verdict, distinct from + # absent, and it is ADVISORY: this gate runs before the organization build + # lock, so it reports rather than blocks until a Windows host demonstrates the + # probe is clean on a healthy toolchain. + Assert-Verdict -Name 'a present compiler that will not start is compiler-unusable' ` + -InstallRoot $root -ExecutablePaths @($missing) -LaunchablePaths @() ` + -ExpectedOk $false -ExpectedReason 'compiler-unusable' -ExpectedAdvisory $true + + # An absent compiler stays `compiler-missing` even with a launch probe wired + # in, so #333's verdict and its blocking failure are unchanged. + Assert-Verdict -Name 'an absent compiler is still compiler-missing with a launch probe' ` + -InstallRoot $root -ExecutablePaths @() -LaunchablePaths @() ` + -ExpectedOk $false -ExpectedReason 'compiler-missing' + + # A newer broken toolset must not hide an older working one, the same way a + # newer absent one already does not. + Assert-Verdict -Name 'falls back past a newer toolset that will not start' ` + -InstallRoot $root -ExecutablePaths @($missing, $older) -LaunchablePaths @($older) ` + -ExpectedOk $true -ExpectedReason 'ok' + + # The three failure messages must stay distinguishable: the operator fix for + # each one differs. + $script:Count++ + $unusable = Test-MsvcToolchain -InstallRoot $root -RunnerName 'TEST-RUNNER' ` + -ProbeExecutable { param($p) $true } -ProbeLaunch { param($p) $false } + if ($unusable.Message -notmatch 'did not start' -or $unusable.Message -notmatch 'TEST-RUNNER') { + Write-Output ("FAIL the compiler-unusable message names the symptom and the runner`n" + + " got $($unusable.Message)") + $script:Failures++ + } + elseif ($VerboseOutput) { Write-Output 'ok the compiler-unusable message names the symptom and the runner' } + + # HOW A LAUNCH OUTCOME IS READ. `cl.exe` with no input files exits non-zero on + # a healthy toolchain, so requiring exit 0 would call a working compiler + # broken. Windows reports a loader failure as an NTSTATUS instead. + $launchCases = @( + @{ Name = 'exit 0 started'; ExitCode = 0; Threw = $false; Expected = $true } + @{ Name = 'exit 2 (no input files) still started'; ExitCode = 2; Threw = $false; Expected = $true } + @{ Name = 'STATUS_DLL_NOT_FOUND did not start'; ExitCode = -1073741515; Threw = $false; Expected = $false } + @{ Name = 'STATUS_INVALID_IMAGE_FORMAT did not start'; ExitCode = -1073741701; Threw = $false; Expected = $false } + @{ Name = 'unsigned NTSTATUS did not start'; ExitCode = 3221225781; Threw = $false; Expected = $false } + @{ Name = 'a throw did not start'; ExitCode = 0; Threw = $true; Expected = $false } + @{ Name = 'no exit code at all did not start'; ExitCode = $null; Threw = $false; Expected = $false } + ) + foreach ($case in $launchCases) { + $script:Count++ + $actual = Test-CompilerLaunchOutcome -ExitCode $case.ExitCode -Threw $case.Threw + if ($actual -ne $case.Expected) { + Write-Output ("FAIL launch outcome: $($case.Name)`n" + + " expected $($case.Expected), got $actual") + $script:Failures++ + } + elseif ($VerboseOutput) { Write-Output "ok launch outcome: $($case.Name)" } + } + # A non-version directory must not crash the comparison. $null = New-Toolset -Version 'not-a-version' Assert-Verdict -Name 'a non-version toolset directory does not break sorting' ` diff --git a/scripts/unity/assert-msvc-toolchain.ps1 b/scripts/unity/assert-msvc-toolchain.ps1 index 920fbcd4..a775f9a6 100644 --- a/scripts/unity/assert-msvc-toolchain.ps1 +++ b/scripts/unity/assert-msvc-toolchain.ps1 @@ -80,15 +80,17 @@ function Test-MsvcToolchain { param( [AllowEmptyString()][string]$InstallRoot, [scriptblock]$ProbeExecutable, + [scriptblock]$ProbeLaunch, [string]$RunnerName = 'unknown' ) if ([string]::IsNullOrWhiteSpace($InstallRoot)) { return [pscustomobject]@{ - Ok = $false - Reason = 'no-visual-studio' - Path = '' - Message = ("No Visual Studio installation with the C++ toolset was found on runner " + + Ok = $false + Advisory = $false + Reason = 'no-visual-studio' + Path = '' + Message = ("No Visual Studio installation with the C++ toolset was found on runner " + "'$RunnerName'. IL2CPP cannot compile a player without it. A runner administrator " + "must install the 'Desktop development with C++' workload.") } @@ -111,33 +113,66 @@ function Test-MsvcToolchain { if ($versions.Count -eq 0) { return [pscustomobject]@{ - Ok = $false - Reason = 'no-toolset' - Path = $toolsRoot - Message = ("Visual Studio is installed on runner '$RunnerName' but carries no MSVC " + + Ok = $false + Advisory = $false + Reason = 'no-toolset' + Path = $toolsRoot + Message = ("Visual Studio is installed on runner '$RunnerName' but carries no MSVC " + "toolset under '$toolsRoot'. IL2CPP cannot compile a player. A runner " + "administrator must add the 'Desktop development with C++' workload.") } } + # A compiler that is present but cannot start is tracked separately from one + # that is absent, because the operator fix differs and because a newer broken + # toolset must not hide an older working one. + $unusable = $null foreach ($version in $versions) { $compiler = Join-Path $version.FullName 'bin\Hostx64\x64\cl.exe' - if (& $ProbeExecutable $compiler) { - return [pscustomobject]@{ - Ok = $true - Reason = 'ok' - Path = $compiler - Message = "MSVC toolset $($version.Name) is usable: $compiler" - } + if (-not (& $ProbeExecutable $compiler)) { continue } + if ($ProbeLaunch -and -not (& $ProbeLaunch $compiler)) { + if ($null -eq $unusable) { $unusable = [pscustomobject]@{ Name = $version.Name; Path = $compiler } } + continue + } + return [pscustomobject]@{ + Ok = $true + Advisory = $false + Reason = 'ok' + Path = $compiler + Message = "MSVC toolset $($version.Name) is usable: $compiler" + } + } + + if ($null -ne $unusable) { + return [pscustomobject]@{ + Ok = $false + # ADVISORY, not blocking. This gate runs before the organization build + # lock, so a false failure would block every IL2CPP leg on the runner. + # No Windows host has yet demonstrated that the launch probe exits + # cleanly on a HEALTHY toolchain invoked by full path outside vcvars + # (#336, step 1), so until it does, a failed launch is reported and the + # leg continues. Flip this to $false in the same change that records + # that demonstration. + Advisory = $true + Reason = 'compiler-unusable' + Path = $unusable.Path + Message = ("MSVC toolset $($unusable.Name) is present on runner '$RunnerName' but " + + "its compiler did not start at '$($unusable.Path)'. A compiler that cannot " + + "start is normally a missing sibling DLL, a truncated file, or security " + + "tooling blocking it, and IL2CPP will fail the player build after taking a " + + "Unity licence seat and the build lock. A runner administrator must repair the " + + "'Desktop development with C++' workload. This is reported, not enforced: see " + + "issue #336.") } } $newest = $versions[0] return [pscustomobject]@{ - Ok = $false - Reason = 'compiler-missing' - Path = (Join-Path $newest.FullName 'bin\Hostx64\x64\cl.exe') - Message = ("MSVC toolset $($newest.Name) is present on runner '$RunnerName' but its " + + Ok = $false + Advisory = $false + Reason = 'compiler-missing' + Path = (Join-Path $newest.FullName 'bin\Hostx64\x64\cl.exe') + Message = ("MSVC toolset $($newest.Name) is present on runner '$RunnerName' but its " + "compiler is not executable at " + "'$(Join-Path $newest.FullName 'bin\Hostx64\x64\cl.exe')'. This is a broken or " + "half-completed Visual Studio update, not a missing install -- Unity will still " + @@ -147,6 +182,40 @@ function Test-MsvcToolchain { } } +function Test-CompilerLaunchOutcome { + <# + .SYNOPSIS + Decide whether a compiler process actually started, from how it ended. + + .DESCRIPTION + Pure, so every branch runs on Linux. The signal is that the process + STARTED, not that it liked its arguments: `cl.exe` invoked with no input + files still exits non-zero on a healthy toolchain, so requiring exit 0 + would call a working compiler broken. + + Windows reports a loader failure -- a missing sibling DLL, a truncated + image, an execution block -- as an NTSTATUS in the exit code rather than + as a compiler diagnostic. `0xC0000135` is STATUS_DLL_NOT_FOUND and + `0xC000007B` is STATUS_INVALID_IMAGE_FORMAT. PowerShell surfaces those as + negative signed integers, so both forms are checked. + #> + [CmdletBinding()] + param( + [AllowNull()][object]$ExitCode, + [bool]$Threw + ) + + if ($Threw) { return $false } + if ($null -eq $ExitCode) { return $false } + + # `0xC0000000L`, with the L. PowerShell parses a hex literal with no type suffix as a + # SIGNED int, so a bare `0xC0000000` is -1073741824 and `0 -ge -1073741824` + # would call every healthy exit code a loader failure. + $code = [int64]$ExitCode + if ($code -lt 0 -or $code -ge 0xC0000000L) { return $false } + return $true +} + # Dot-sourced by the tests, which supply their own inputs. if ($MyInvocation.InvocationName -eq '.') { return } @@ -221,23 +290,37 @@ else { "with C++' workload.") } -# Presence, deliberately -- NOT a launch probe. Running `cl.exe /?` would also -# catch a present-but-corrupt compiler, which this cannot. It was considered and -# rejected for now: `cl.exe` resolves several sibling DLLs (`mspdb*`, `msvcp*`) -# and the toolchain normally runs under `vcvars`, so a bare launch has real ways -# to fail on a HEALTHY host -- and this gate runs before the organization build -# lock, so a false failure blocks every IL2CPP leg on that runner. That is the -# same shape as the `vswhere` bug above, where a discovery failure was reported -# as a missing toolchain. +# Presence AND a launch probe. Presence is what #333 needed and is the blocking +# check. The launch probe catches a `cl.exe` that is there but cannot start -- a +# failed update, a missing sibling DLL, security tooling -- which presence cannot +# see and which today reports the host healthy and fails the leg twenty minutes +# later, after taking a licence seat and the build lock. # -# The asymmetry decides it: a missing `cl.exe` is what #333 actually was, and -# presence catches it. A corrupt-but-present compiler is rarer and still fails -# the build, just later. Adding a launch probe needs a Windows host where both -# the healthy and the corrupt case can be demonstrated, which is #336, not a -# change made blind. (GitHub Copilot raised this.) +# The launch verdict is ADVISORY. This gate runs before the organization build +# lock, so a launch probe that is wrong about a HEALTHY host would block every +# IL2CPP leg on that runner. That risk is real: `cl.exe` resolves sibling DLLs +# from its own directory and the toolchain normally runs under `vcvars`, and no +# Windows host has yet demonstrated the healthy case (#336, step 1). Reporting it +# costs nothing and names the cause up front; enforcing it needs that +# demonstration first. $result = Test-MsvcToolchain -InstallRoot $installRoot -RunnerName $runner -ProbeExecutable { param($path) Test-Path -LiteralPath $path -PathType Leaf +} -ProbeLaunch { + param($path) + # EVERYTHING is inside the try, including reading the outcome. This verdict is + # advisory, so an unexpected throw here must become "did not start" and a + # warning, never a terminating error that fails the leg this gate exists to + # protect. + try { + # By full path, with no input files, output discarded: the question is + # whether the image loads, not what it says. + $null = & $path '/?' 2>&1 + return (Test-CompilerLaunchOutcome -ExitCode $LASTEXITCODE -Threw $false) + } + catch { + return (Test-CompilerLaunchOutcome -ExitCode $null -Threw $true) + } } if ($result.Ok) { @@ -245,8 +328,8 @@ if ($result.Ok) { exit 0 } -if ($DetectOnly) { - Write-Output "::warning::$($result.Message)" +if ($DetectOnly -or $result.Advisory) { + Write-Output "::warning title=MSVC C++ toolchain ($($result.Reason))::$($result.Message)" exit 0 } diff --git a/scripts/unity/run-ci-tests.ps1 b/scripts/unity/run-ci-tests.ps1 index f452d41c..3496adb8 100644 --- a/scripts/unity/run-ci-tests.ps1 +++ b/scripts/unity/run-ci-tests.ps1 @@ -498,6 +498,85 @@ function Get-NUnitNodeFullName { # cap). Attribute reads use XmlElement.GetAttribute (returns '' when absent, # never throws) so a results.xml lacking a fullname/name attribute does NOT # degrade the whole enumeration to a generic warning under Set-StrictMode. +function Write-SuiteWallClockSummary { + <# + .SYNOPSIS + Lift the suite's own wall-clock line out of the Unity log and into the job summary. + + .DESCRIPTION + Issue #410: a change added 78 seconds to the EditMode step on every editor + leg and stayed green for two days, because nothing in CI looks at how long + a step takes. `SuiteWallClockBudgetTest` already measures the suite and + already warns past its soft budget, but only into the Unity log, which + nobody reads on a green run. + + Printing the number it already has costs one regex per leg and needs no + new script and no new workflow, which is what issue #410 asks for. It does + not compare against history: that is the option the issue calls most at + odds with the repository's tooling philosophy, and it is not taken here. + #> + [CmdletBinding()] + param( + [string]$LogPath, + [Parameter(Mandatory = $true)][string]$Label + ) + + if (-not $LogPath -or -not (Test-Path -LiteralPath $LogPath -PathType Leaf)) { + return + } + + try { + # The producing side formats with the invariant culture, so the decimal + # separator here is always '.'. + $pattern = 'DxMessaging suite wall clock:\s*([0-9.]+)s\s*\(soft budget\s*([0-9.]+)s,\s*hard budget\s*([0-9.]+)s' + $match = @( + Select-String -LiteralPath $LogPath -Pattern $pattern -ErrorAction SilentlyContinue | + Select-Object -Last 1 + ) + if ($match.Count -lt 1) { + return + } + + $groups = $match[0].Matches[0].Groups + $invariant = [System.Globalization.CultureInfo]::InvariantCulture + $elapsed = 0.0 + $soft = 0.0 + if (-not [double]::TryParse($groups[1].Value, 'Float', $invariant, [ref]$elapsed)) { return } + if (-not [double]::TryParse($groups[2].Value, 'Float', $invariant, [ref]$soft)) { return } + + $summaryPath = $env:GITHUB_STEP_SUMMARY + if ($summaryPath) { + # The header is looked up in the FILE, not held in a variable: the + # workflow runs this script once per test mode, so each leg is its own + # pwsh process and an in-process flag would print the header three + # times per job. + $header = '### Suite wall clock' + $alreadyOpen = (Test-Path -LiteralPath $summaryPath -PathType Leaf) -and + @(Select-String -LiteralPath $summaryPath -Pattern ([regex]::Escape($header)) ` + -SimpleMatch:$false -ErrorAction SilentlyContinue).Count -gt 0 + if (-not $alreadyOpen) { + Add-Content -LiteralPath $summaryPath -Value @( + $header, + '', + '| Leg | Elapsed | Soft budget | Hard budget |', + '| --- | ---: | ---: | ---: |' + ) + } + Add-Content -LiteralPath $summaryPath -Value ("| $Label | $($groups[1].Value)s | " + + "$($groups[2].Value)s | $($groups[3].Value)s |") + } + + if ($elapsed -gt $soft) { + Write-Host ("::warning::${Label} suite wall clock $($groups[1].Value)s is over its " + + "$($groups[2].Value)s soft budget (hard budget $($groups[3].Value)s). A step that " + + "grew without breaching its ceiling is what issue #410 was raised for.") + } + } catch { + # Best-effort reporting must never mask a real result. + Write-Host "::warning::Could not read the suite wall clock for ${Label}: $($_.Exception.Message)" + } +} + function Write-UnityFailedTestAnnotations { [CmdletBinding()] param( @@ -3304,6 +3383,7 @@ function Test-NUnitResults { $skipped = [int]$run.skipped Write-Host "Results: total=$total passed=$passed failed=$failed skipped=$skipped" + Write-SuiteWallClockSummary -LogPath $LogPath -Label $Label if ($total -lt 1) { Write-CiError "0 tests ran for $Label -- check assembly selection and package testables.$exitNote" throw "0 tests ran for $Label." diff --git a/scripts/validate-js-loc-budget.js b/scripts/validate-js-loc-budget.js index 466a9ee5..99234182 100644 --- a/scripts/validate-js-loc-budget.js +++ b/scripts/validate-js-loc-budget.js @@ -120,7 +120,21 @@ const path = require("path"); // false red. +29 source and +59 test lines, the tests covering a live // editor, a stale discovery record, a reply carrying no editor state, an // empty reply, and both no-call paths: 18181. -const TOTAL_BUDGET = 18181; +// 081 Make the suite wall clock visible in the job summary (#410). A change +// added 78 seconds to the EditMode step on every editor leg and stayed +// green for two days, because nothing in CI reads how long a step takes. +// `SuiteWallClockBudgetTest` already measures the suite and already warns +// past its soft budget, into a log nobody opens on a green run. The CI +// harness now lifts that one line into the job summary and warns when it is +// over. +168 test lines covering under and over budget, the once-per-job +// table header, a log with no line, a missing log, and the shared line +// shape between the C# producer and the PowerShell consumer. No new script +// and no new workflow, which is the option issue #410 asks for, plus the +// 11 lines this entry itself adds. Review then moved the once-per-job header +// check into the summary FILE, because the workflow runs the harness once +// per test mode, so an in-process flag printed the header three times per +// job; the test now spawns one process per leg: 18367. +const TOTAL_BUDGET = 18367; const LARGEST_FILE_COUNT = 10; const REPO_ROOT = path.resolve(__dirname, "..");