Skip to content

Remove assert usage from internal/replacer #3236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DrJosh9000 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove assert usage from internal/replacer #3236

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions internal/replacer/replacer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ import (
"bytes"
"fmt"
"io"
"slices"
"strings"
"testing"

"github.com/buildkite/agent/v3/internal/redact"
"github.com/buildkite/agent/v3/internal/replacer"
"github.com/google/go-cmp/cmp"
"gotest.tools/v3/assert"
"github.com/google/go-cmp/cmp/cmpopts"
)

const lipsum = "Lorem ipsum dolor sit amet"
Expand Down Expand Up @@ -302,31 +301,42 @@ func TestReplacerMultiLine(t *testing.T) {
func TestAddingNeedles(t *testing.T) {
t.Parallel()

needles := []string{"secret1111", "secret2222"}
afterAddExpectedNeedles := []string{"secret1111", "secret2222", "pre-secret3333"}
sortSlices := cmpopts.SortSlices(func(a, b string) bool { return a < b })

var buf strings.Builder
replacer := replacer.New(&buf, needles, redact.Redact)
actualNeedles := replacer.Needles()
replacer := replacer.New(&buf, []string{"secret1111", "secret2222"}, redact.Redact)
gotNeedles := replacer.Needles()
wantNeedles := []string{"secret1111", "secret2222"}

slices.Sort(needles)
slices.Sort(actualNeedles)
assert.DeepEqual(t, actualNeedles, needles)
if diff := cmp.Diff(gotNeedles, wantNeedles, sortSlices); diff != "" {
t.Errorf("replacer.Needles() diff (-got +want):\n%s", diff)
Copy link
Member

@pda pda Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you use an editor snippet or something for this?
It's quite fiddly to type out many times 😅

Copy link
Member

@pda pda Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see https://google.github.io/styleguide/go/decisions#print-diffs doesn't have a strong opinion on the order of (got, want) vs (want, got).

Here's an example from another test I'm trying this style on, where I used the same (got, want) order you've used here:

    env_test.go:44: env.Get() diff (-got +want):
          string(
        -       "intrestng",
        +       "interesting",
          )

To me that diff reads backwards… I'd expect the + to be showing me what it got, as a delta against what it wanted. (Perhaps there's also an argument for the other direction.) I guess I think of the - side of a diff as the older/control side, and the + side as the newer/experiment/work-in-progress side.

I do note that the first (primary?) example in that style guide does use diff (-want +got) which matches what makes sense to me.

Any thoughts?

Copy link
Contributor Author

@DrJosh9000 DrJosh9000 Mar 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you use an editor snippet or something for this?
It's quite fiddly to type out many times 😅

I don't, but I should look into using some IDE feature for this. As fiddly as it is to write, I think it pays for itself in time spent writing out.

opinion on the order of (got, want) vs (want, got)

I've seen a few discussions on this as the print-diffs guidance was written 😆 Before it became common, I already had "got before want" drilled into my brain. Someone (I forget who) raised the idea that (-got, +want) is a diff describing "here's what would need to change in the output to match the test", and the original stance (which was to prefer (-want +got)) was weakened to "make the diff direction clear".

}

_, err := replacer.Write([]byte("redact secret1111 and secret2222 but not pre-secret3333\n"))
assert.NilError(t, err)
input1 := "redact secret1111 and secret2222 but not pre-secret3333\n"
if _, err := replacer.Write([]byte(input1)); err != nil {
t.Errorf("replacer.Write(%q) error = %v", input1, err)
}

replacer.Add("pre-secret3333")
actualNeedles = replacer.Needles()
slices.Sort(actualNeedles)
slices.Sort(afterAddExpectedNeedles)
assert.DeepEqual(t, actualNeedles, afterAddExpectedNeedles)
gotNeedles = replacer.Needles()
wantNeedles = []string{"secret1111", "secret2222", "pre-secret3333"}

_, err = replacer.Write([]byte("now redact secret1111, secret2222, and pre-secret3333\n"))
assert.NilError(t, err)
assert.NilError(t, replacer.Flush())
if diff := cmp.Diff(gotNeedles, wantNeedles, sortSlices); diff != "" {
t.Errorf("replacer.Needles() diff (-got +want):\n%s", diff)
}

assert.Equal(t, buf.String(), "redact [REDACTED] and [REDACTED] but not pre-secret3333\nnow redact [REDACTED], [REDACTED], and [REDACTED]\n")
input2 := "now redact secret1111, secret2222, and pre-secret3333\n"
if _, err := replacer.Write([]byte(input2)); err != nil {
t.Errorf("replacer.Write(%q) error = %v", input2, err)
}
if err := replacer.Flush(); err != nil {
t.Errorf("replacer.Flush() = %v", err)
}

got, want := buf.String(), "redact [REDACTED] and [REDACTED] but not pre-secret3333\nnow redact [REDACTED], [REDACTED], and [REDACTED]\n"
if diff := cmp.Diff(got, want); diff != "" {
t.Errorf("replacer output diff (-got +want):\n%s", diff)
}
}

func BenchmarkReplacer(b *testing.B) {
Expand Down