test: add test for options env

[BrunoQuaresma]

After some thinking, I concluded that we don't need to test all the env vars but how those values are parsed, but let me know if you think we should test all the env variables.

Closes #157

[dannykopping]

The previous code was using https://pkg.go.dev/strconv#ParseBool, but I'm not sure what the new code is using so we might need to test 1/0/true/false (and the latter two's capitalised equivalents).

[Emyrk]

The runCLI is doing a lot of extra work, where this is really just testing env var assignment. An option instead is to skip the cli part, and just do the env parsing:

	t.Run("string", func(t *testing.T) {
		var o envbuilder.Options
		set := o.CLI()
		err := set.ParseEnv([]serpent.EnvVar{
			{"SETUP_SCRIPT", "setup.sh"},
			{"INIT_COMMAND", "sleep infinity"},
		})
		require.NoError(t, err)

		require.Equal(t, o.SetupScript, "setup.sh")
		require.Equal(t, o.InitCommand, "sleep infinity")
	})

If we do this, we can also just merge all the tests. But one thing that feels off here is that we are only testing the happy paths, which makes this test feel weak in general.

We could comment this is just a sanity check to make sure we don't change env vars without documenting it to prevent any breaking changes. In that case, I think merging the tests and doing it the method above would be a bit cleaner.

// TestEnvOptionParsing prevents breaking changes to the environment variables.
func TestEnvOptionParsing(t *testing.T) {
	t.Parallel()

	var o envbuilder.Options
	set := o.CLI()
	err := set.ParseEnv([]serpent.EnvVar{
		{"SETUP_SCRIPT", "setup.sh"},
		{"INIT_COMMAND", "sleep infinity"},
		// ... Add the rest
		{"CACHE_TTL_DAYS", "7"},
	})
	require.NoError(t, err)

	require.Equal(t, o.SetupScript, "setup.sh")
	require.Equal(t, o.InitCommand, "sleep infinity")
 	// ... Add the rest
	require.Equal(t, o.CacheTTLDays, int64(7))
}

[BrunoQuaresma]

@Emyrk I think it makes sense. Since it is a straightforward test I don't see how to test "unhappy" paths, how would you do that in this scenario?

[Emyrk]

@Emyrk I think it makes sense. Since it is a straightforward test I don't see how to test "unhappy" paths, how would you do that in this scenario?

I don't think it is worth it to test unhappy paths, it does feel excessive. Just trying to determine what value this test brings. We do not add any validation I can see to the options either, so unhappy paths like negative numbers, or urls are not caught at this layer.

[dannykopping]

Almost there, thanks @BrunoQuaresma 👍

[Emyrk]

This looks fine to me. I think we should port a more rigorous test like this to the serpent package.

Side note, do we not have a linter on this project?

[dannykopping]

Good work @BrunoQuaresma! Thanks for being patient with the review.