Show every region that differs when Should-BeString fails on a big string - #3010
Merged
Conversation
…ring Should-BeString printed both strings in full when they were different. For the 10 000 line file in pester#2951 that is a hundred thousand lines of output and a character offset nobody can act on. Scanning for the first differing line fixes that when one thing changed. It does not fix comparing a snapshot, where several things change at once and the useful output is every changed region with its context. A scan stops at the first one. So the diffing is done by DiffPlex now, and Pester renders it: - Strings small enough to read are left exactly as they were, the full text with a caret under the difference is the most precise thing we can show. - Bigger ones get the changed regions with two lines of context each, expected line numbers on the left and actual on the right, at most five regions and a count of how many were left out. - Only the lines that differ are expanded, so a trailing space or a stray CR is visible without turning every other line into escape codes. Trailing spaces show up as the middle dot, control characters keep the Control Pictures that Formatter.EscapeControlChars already uses. - When every line has the same text and only the endings differ it says so and names the endings on each side, because printing the lines would show two blocks that look identical. Two DiffPlex defaults are wrong for this and both of them fail quietly. ignoreWhitespace defaults to true, and with it a difference that is only a trailing space is reported as no difference at all. The default line chunker throws the endings away, so CRLF against LF comes back as two identical strings. StringDiff.cs sets both, and there are tests for both, because a regression there is silent. DiffPlex is vendored, not referenced. Shipping DiffPlex.dll next to Pester.dll puts a second assembly identity in the session, and Windows PowerShell 5.1 has no load context isolation, so another module that already loaded a different DiffPlex version wins. 19 files, 1080 lines, Apache-2.0 which is the same licence Pester uses, and the only change to them is the namespace. VENDORING.md records the version and Update-VendoredDiffPlex.ps1 re-copies it, or checks it with -Verify. It costs 19 KB of Pester.dll on net8.0 and 18.5 KB on net462, against 33 KB per target framework for the full library. Fix pester#2951 Fix pester#3006 🤖
There was a problem hiding this comment.
PSScriptAnalyzer found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
The first commit put DiffPlex's LICENSE.txt next to the vendored source and stated the one modification in VENDORING.md. That covers the repository. It does not cover two things Apache-2.0 asks for. Section 4(b) asks that modified files say they are modified. All 19 files are modified, the namespace is Pester.DiffPlex, and none of them said so. They now carry a four line header with the source, the author, the licence and the change. The upstream files have no copyright header of their own, so the same header does the attribution, which the nuspec gives as Matthew Manela. Section 4(a) asks that whoever gets the code gets the licence, in source or in binary form. Pester.dll has DiffPlex compiled into it and the built module shipped no licence file at all, so anyone installing from the gallery was getting DiffPlex without it. The module now ships ThirdPartyNotices.txt, and build.ps1 copies it in. Update-VendoredDiffPlex.ps1 takes the header off before comparing, so -Verify still proves every file is upstream apart from the namespace. The recorded commit is now 3cb6415, which is what the 1.9.0 nuspec says the package was built from. The DiffPlex folder there is identical to 8821ff9, which is what I recorded before, so the vendored code did not change. The headers cost bytes because the assemblies are built with embedded symbols. Pester.dll grows by 24 KB per target framework now, not the 19 KB the first commit reported. 🤖
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Vendoring put 20 DiffPlex types into Pester's public API. Nothing asked for that, and it would make the vendored library part of the compatibility surface, so replacing it or updating it across a breaking change becomes our problem. Every vendored type is internal now. Pester exposes one method, Pester.StringDiff.Format, which takes the two strings and returns the part of the failure message that describes how they differ. StringDiffResult and Compare are internal, and PesterTests sees them through InternalsVisibleTo. The files were already modified, so there was no reason to keep code we never call. pester.patch removes Differ's convenience methods, IDiffer, ISideBySideDiffBuilder, and the SideBySideDiffBuilder constructors and static helpers that default to LineChunker. Five files then have nothing referring to them and are not vendored at all. 19 files became 14, and Pester.dll grows by 22 KB per target framework instead of 24 KB. WordChunker and DelimiterChunker stay. SideBySideDiffBuilder uses them for the word level sub pieces on a changed line, which we do not render yet, and highlighting the changed part inside a line is the obvious next thing to do to this message. Cutting them means editing the core of SideBySideDiffBuilder rather than deleting whole members. Deleting code is what breaks a vendored copy, because a hand-trimmed one diverges quietly and nobody can tell what was changed on purpose. So the removals are kept as a patch, not applied by hand. Update-VendoredDiffPlex.ps1 rebuilds the whole thing from upstream, applies the namespace rewrite, the internal rewrite and the header, then applies pester.patch, and -Verify compares that against what is on disk. Running it against a newer DiffPlex either works or git says which hunk failed. -Regenerate rewrites the patch after a deliberate hand edit. 🤖
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.
Supersedes #3003, which scanned for the first differing line. That answers "one thing changed". Comparing a snapshot is "many things changed", and a scan stops at the first one.
Fix #2951
Fix #3006
What it prints
Four lines changed in a
package.json:Expected line numbers on the left, actual on the right, so it stays readable when lines are added and the two sides stop lining up:
Only the lines that differ are expanded, so a tab or a trailing space is visible without turning the context into escape codes:
The one difference you cannot work out by reading the output gets named instead:
At most five regions are printed, then it says how many are left:
Short strings are untouched. The full text with a caret under the difference is still the most precise thing we can show, and a long single line still gets the excerpt with ellipses.
Two DiffPlex defaults that fail quietly
Both are set in
StringDiff.csand both have tests, because getting them wrong produces no output rather than wrong output.ignoreWhitespacetrueLineChunkerLineEndingsPreservingChunkerkeeps them.SideBySideDiffBuilderpairs changed lines and gives positions on both sides, so Pester does not have to do that bookkeeping.Public surface: one method
Pester.StringDiff.Format(expected, actual, caseSensitive, context, maxRegions)returns the part of the failure message that describes how the two strings differ. That is the whole public API this PR adds.Every vendored type is
internal, so DiffPlex is not part of Pester's compatibility surface and can be updated, trimmed further or swapped out without that being a breaking change.StringDiffResultandCompareare internal too; the unit tests reach them throughInternalsVisibleTo.Vendored, not referenced
Shipping
DiffPlex.dllnext toPester.dllputs a second assembly identity in the user's session. Windows PowerShell 5.1 has no load context isolation, so a module that already loaded a different DiffPlex version wins and Pester cannot do anything about it.ThreeWayDiffer,UnidiffRendererandInlineDiffBuilderare left behind, and so are five more files that only existed to serve members Pester never calls.internal, licence header) pluspester.patch, which removes the members Pester does not call. The patch touches two files, removes 99 lines and adds 7.src/csharp/Pester/DiffPlex/VENDORING.mdrecords the version and the commit (3cb6415, what the 1.9.0 nuspec says the package was built from), and says what is left out and why.Update-VendoredDiffPlex.ps1rebuilds the whole copy from upstream and-Verifycompares the result against what is on disk. Deleting code is what usually turns a vendored copy into a fork nobody can update, so the removals are kept as a patch rather than applied by hand: run it against a newer DiffPlex and either the patch applies or git says which hunk failed.The licence obligations
Apache-2.0 asks for three things and the second commit adds two of them.
src/csharp/Pester/DiffPlex/LICENSE.txt, unmodifiedThirdPartyNotices.txt, which build.ps1 now copies into the built module.Pester.dllhas DiffPlex compiled into it, and the module previously shipped no licence file at allThere is no NOTICE file upstream, so section 4(d) does not apply. The upstream source files carry no copyright header of their own, so there was nothing to retain under 4(c); the header does the attribution instead, naming Matthew Manela as the nuspec does.
Update-VendoredDiffPlex.ps1 -Verifystrips the header before comparing, so it still proves every file is upstream apart from the namespace.Upstream releases about once a year (1.7.2 in 2023-12, 1.8.0 in 2025-05, 1.9.0 in 2025-09), so this is not something we need to track closely.
Cost with embedded symbols, which is how
Pester.csprojbuilds:Pester.dllnet8.0Pester.dllnet462Embedded symbols carry the source text, so the licence headers cost about 5 KB and dropping the unused members gave 3.5 KB back.
The full
DiffPlex.dllis 33.3 KB per target framework, so vendoring the part we use is also the smaller option.Verification
src/csharp/PesterTests: 78 passed, 0 failed. 20 of those are new, inStringDiffTests.cs.Update-VendoredDiffPlex.ps1 -Verifypasses against DiffPlex3cb6415.Note that CI does not currently run
src/csharp/PesterTestsat all, so the C# tests here only run locally. That is worth fixing separately.🤖