diff: add package, make testscript use it #205

mvdan · 2023-03-22T17:16:58Z

(see commit messages - please do not squash)

Fixes #157.

From Go tip as of March 21st 2023, at commit 5f1a0320b92a60ee1283522135e00bff540ea115. The only change is to replace the internal/txtar dependency with our own txtar package. It seems like upstream has its own tiny copy of x/tools/txtar, presumably so that even low level packages can use txtar in tests. Fixes rogpeppe#157.

The main reason to prefer a copy of Go's internal/diff over pkg/diff is that internal/diff is much more efficient in both time and memory usage. In particular, pkg/diff required quadratic space in memory, which could easily cause "out of memory" errors in Go tests per pkg/diff#26. Beyond making the `cmp` command better able to handle large files, this also moves us back to having zero external dependencies, which is always a nice to have. The long_diff test still appears to work well; the output is changed since the new package produces a shorter, but still entirely correct, diff. It also seems like the new package includes a leading "diff" line to show the two filenames. That seems like a harmless change.

myitcv

LGTM - but would also ask @rogpeppe to confirm the change in package "API" surface.

mvdan · 2023-03-22T17:40:23Z

CC @thepudds

mvdan · 2023-03-22T17:41:03Z

Roger agreed in #157 (comment), and again in voice, so I'm merging :)

thepudds · 2023-03-22T17:51:25Z

Hi @mvdan , looks great, and thanks! (Sorry, this was still on my to do list, but not my "Burning Must Do Now" list 😅 ).

Would it make sense to remove or modify as well (currently still there on master):

go-internal/testscript/cmd.go

Lines 144 to 152 in 50a1440

    
           // pkg/diff is quadratic at the moment. 
        
           // If the product of the number of lines in the inputs is too large, 
        
           // don't call pkg.Diff at all as it might take tons of memory or time. 
        
           // We found one million to be reasonable for an average laptop. 
        
           const maxLineDiff = 1_000_000 
        
           if strings.Count(text1, "\n")*strings.Count(text2, "\n") > maxLineDiff { 
        
           	ts.Fatalf("large files %s and %s differ", name1, name2) 
        
           	return 
        
           }

mvdan · 2023-03-22T17:56:11Z

Oh my, how did I miss that? Happy to approve and merge a PR if you send one, otherwise I'll send one in a bit and you can approve.

mvdan added 2 commits March 22, 2023 17:15

mvdan requested review from rogpeppe and myitcv March 22, 2023 17:16

myitcv approved these changes Mar 22, 2023

View reviewed changes

mvdan merged commit 50a1440 into rogpeppe:master Mar 22, 2023

mvdan deleted the diff-testscript branch March 27, 2023 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff: add package, make testscript use it #205

diff: add package, make testscript use it #205

mvdan commented Mar 22, 2023

myitcv left a comment

mvdan commented Mar 22, 2023

mvdan commented Mar 22, 2023

thepudds commented Mar 22, 2023

mvdan commented Mar 22, 2023

diff: add package, make testscript use it #205

diff: add package, make testscript use it #205

Conversation

mvdan commented Mar 22, 2023

myitcv left a comment

Choose a reason for hiding this comment

mvdan commented Mar 22, 2023

mvdan commented Mar 22, 2023

thepudds commented Mar 22, 2023

mvdan commented Mar 22, 2023