[feature] Reference testing.T once only

[sebdah]

This change makes the reference to *testing.T required only when calling goldie.New(). For the sub-sequent Assert*(), the passing of *testing.T is not required.

This makes the users code less verbose as fewer parameters are required for the assertions.

[sebdah]

@novas0x2a / @gtrevg: Do you guys have any thoughts on this change? Basically we'd move from:

g := goldie.New(t)
g.Assert(t, ...)

to

g := goldie.New(t)
g.Assert(...)

[gtrevg]

@sebdah The main reason for originally passing in t into goldie.New(t) was so that, if there were any initialization errors, it would would fail the test and not proceed.

        if err != nil {
            t.Error(fmt.Errorf("Could not apply option: %w", err))
            t.FailNow()
        }

This was one pattern I was using a lot in my testing:

func TestMyStuff(t *testing.T) {
    g := golden.New(
        t,
        golden.WithFixtureDir(filepath.Join(testingDefaultFixtureDir, "golden")),
        golden.WithSubTestNameForDir(true),
        golden.WithTestNameForDir(true),
    )

     // Oftentimes I am pulling test data from the file system and can
    // fast fail on `t` if there's an issue.
    testCases := getTestCases(t) 

    for i, tc := range testCases {
        t.Run(tc.Name, func(t *testing.T) {
            // do some  stuff
            result := doSomeStuff(tc.Data)
            g.Assert(t, tc.Name+"_stuff", result)

            // do some other  stuff
            result := doSomeOtherStuff(tc.Data)
            g.Assert(t, tc.Name+"_otherstuff, result)
        })
    }
}

The impact of this change (as can be seen from the updated unit test) is that the goldie.New() function now has to live within the loop. From a visual/code organization standpoint, you end up having to worry about setup much deeper in the testing code.

In addition, if you end up running through more than one subtest loop in your test, you'll have more than one section where you're initializing goldie. If you have several configuration options for goldie, this would end up being much more verbose and error prone than passing in t. This is a tradeoff for passing one less parameter into Assert().

func TestMyStuff(t *testing.T) {

     // Oftentimes I am pulling test data from the file system and can
    // fast fail on `t` if there's an issue.
    testCases := getTestCases(t) 

        someApp := NewApp()

    for i, tc := range testCases {
        t.Run(tc.Name, func(t *testing.T) {

            g := golden.New(
                t,
                golden.WithFixtureDir(filepath.Join(testingDefaultFixtureDir, "golden")),
                golden.WithSubTestNameForDir(true),
                golden.WithTestNameForDir(true),
            )

            // do some  stuff
            result := someApp.AddItem(tc.Data)
            g.Assert(tc.Name, result)
    }

    someApp.ProcessAndSortData()
    processedData := someApp.GetAllTheProcessedData()

    for _, data := range processedData {
        t.Run(data.Name, func(t *testing.T) {

            g := golden.New(
                t,
                golden.WithFixtureDir(filepath.Join(testingDefaultFixtureDir, "golden")),
                golden.WithSubTestNameForDir(true),
                golden.WithTestNameForDir(true),
            )

            g.Assert(data.Name + "_original", data)
            g.Assert(data.Name + "_pruned", pruneData(data))
        })
    }

}

This only becomes an issue if you want to leverage golden.WithSubTestNameForDir(true). Otherwise, I think one would initialize goldie outside the subtest loop and there would not be the issue of initialization duplication.

I personally lean towards more setup in the beginning of a test and then focus more on the testing goal in the latter part of the test.

[sebdah]

I personally lean towards more setup in the beginning of a test and then focus more on the testing goal in the latter part of the test.

I think this is a fair point that is probably quite universally true.

Maybe in fact what should be changed is New. If we don't pass *testing.T there, we give more control to the user. Right now we will fail the test if an option is incorrect. Perhaps returning an error would be better;

func New(t *testing.T, options ...Option) (*goldie, error) {
    g := goldie{
        fixtureDir:     defaultFixtureDir,
        fileNameSuffix: defaultFileNameSuffix,
        filePerms:      defaultFilePerms,
        dirPerms:       defaultDirPerms,
    }

    for _, option := range options {
        err := option(&g)
        if err != nil {
            return nil, fmt.Errorf("Could not apply option: %w", err)
        }
    }

    return &g, nil
}

[gtrevg]

In regards to the idea of taking in a t *testing.T, I took the idea from Mitchell Hashimoto's blog post and consider goldie a test helper:

• Never return errors. Just pass in *testing.T and fail.
• By not returning errors, usage is much prettier, since you don't have a bunch of visual overhead from error handling code in your tests since error checking is gone.
• Used to make tests clear on what they’re testing vs what is boilerplate

So then rather than having to remember to check for errors, I always go with the expectation that if something goes critically wrong (like honoring a requested option), that there's probably some sort of programming error and the test should just fail. An example could be if goldie validated the fixtureDir to ensure that some file system incompatible characters weren't passed in (like a newline or something).

[sebdah]

Hmm, interesting. That's a good bed time story. I'll read and think about that. Good discussion :+1:

[gtrevg]

Heheheh. It is :)

[sebdah]

Alright, I learned something on this. Thanks for the good discussion, I think what you say make a lot of sense! I'll just close this PR.

Regarding the v2.0.0 release of goldie, I'm gonna write a note about this release breaking backwards compatibility, tag the release and ship it. Hopefully today.