Fix bug where env export had trailing \r on windows

[lox]

See bug reported in #569.

[lox]

Apologies for the large diff, I took the opportunity to cleanup the test. I've been moving towards more of the table-based tests and moving away from testing helpers in favor of vanilla golang. Feels @keithpitt?

[keithpitt]

The bug fix looks good to me. My only gripe about the built-in Golang stuff is that it means you just need more boilerplate to test stuff.

	assertEqualEnv(t, "DOLLARS", "i love $money", env)
 	assertEqualEnv(t, "WITH_NEW_LINE", `i have a \n new line`, env)
	assertEqualEnv(t, "CARRIAGE_RETURN", `i have a \r carriage`, env)
	assertEqualEnv(t, "TOTES", `with a " quote`, env)

...becomes...

	for k, expected := range map[string]string{
		`DOLLARS`:         `i love $money`,
		`WITH_NEW_LINE`:   `i have a \n new line`,
		`CARRIAGE_RETURN`: `i have a \r carriage`,
		`TOTES`:           `with a " quote`,
	} {
		val, _ := env.Get(k)
		if val != expected {
			t.Fatalf("Expected %s to be %q, got %q", k, expected, val)
		}
	}

And because it's more verbose, you tend more towards meta-programming tests with arrays and stuff, instead of just keeping it simple with extra lines.

There's just more mental overhead when writing a pure vanilla test, than just using the helpers. My other gripe is that you have to write your own error messages, which just makes writing tests that little more annoying. And if tests are annoying to write, you end up writing less of them.

My feels are for keeping the helpers, and try to keep as little boilerplate code out of tests as possible. They should be as "put goes in, test data out" as possible. And since theres less code, there's less things to think about when it comes to debugging tests "maybe it's my test code that's wrong! Whoops, I forgot to loop over this table thingy".

In saying all that though, I'm happy to be convinced that vanilla is better! What are the benefits of the pure vanilla approach?

[keithpitt]

Oh, and my other argument against more boilerplate in tests, is that the tests become more about the boilerplate code, and less about the test cases. When I read a vanilla test, my brain has to parse (how) your testing it, then the input/output data.

[lox]

TL;DR: Cool, I will bring back the testify assertions and play around with whether the table based tests make sense here.

Three different things in play here, which need addressing differently.

1. More specific test names.

The previous code had test names like TestFromExport with lots of examples broken up into comma delimited headers. At a glance, it's really hard to figure out what each thing is testing, and it's hard to quickly grok what has broken when a test fails.

To me, refactoring to more specific test names is super important. If TestFromExportHandlesNewlines fails, or TestFromExportHandlesEscapedCharacters, I know quickly what is wrong. Beyond that, test cases are nice and short and make the code more approachable.

2. Test helpers

This one is more tricky, I waver on whether to give in to testify. The example you posted is an excellent one though. The reason I made that change was that env.Get was refactored to return a second parameter, so it couldn't simply be passed into the asserts. I decided a table driven test was the lesser of two evils. But let's assume that wasn't the case and that I'd simply rewritten it from the helper. My take is on that is:

a) It's super obvious what is happening, you don't need to know what assertEqualEnv does, specifically whether it will fail the test or just print an error (have run into that bug with several assert.NotNil from around the place). Because all the code is just stock standard testing calls, you don't run into confusion around ordering of parameters either. I see this as a core bit of golang, a trade-off of verbosity for clarity in situations like this.

b) It's idiomatic go, and the way the standard library writes tests, so once you are familiar with it, there are a lot of upsides. That applies to table driven tests too.

c) You don't have to bring in an entire heavy dependency just for some slightly better assertion methods. I hate having to decide whether it's worth bringing in testify in new projects or smaller tests.

The downside is that sometimes it's slightly more verbose. I do pine for a good assert.Equals function as part of the standard library, but I'm just reluctant to deal with all the other downsides of testify.

Beyond that, I think that using testing libraries tends to lead to using more and more of the library, at which point it becomes less and less go-like and harder to use the neat stuff that comes in each golang release (table driven tests!)

3. Table driven tests

I think cases where you are testing a ton of different inputs and checking they match different outputs make for great table driven tests. My view is they are more clearly declarative in that you see a bit list up front of things to test, and you feel more confident adding new tests. I overuse them sometimes, but generally I'm a fan. I like that you can use the new Subtest stuff so that the different cases you are testing are included in the test name.

An example where I think it worked really well is here: https://github.com/buildkite/agent/blob/master/bootstrap/integration/hooks_integration_test.go#L224

[lox]

My other gripe is that you have to write your own error messages

That is probably my leading golang gripe FWIW.

[lox]

Doh! cannot find package "github.com/stretchr/testify/require"

My other pet peeve. A whole different package for assertions that should fail fast.

[keithpitt]

Those are some good counter points. More specific test case names I think is a #goodidea so that I'm totally onboard with.

I'd actually argue that the grid here:

https://github.com/buildkite/agent/blob/master/bootstrap/integration/hooks_integration_test.go#L234

Actually doesn't tell a great story about what it's testing. You have to read the surrounding code to understand what each boolean means. Once you realise what's going on, you need to reverse engineer the co-ordinates of each true/false back into what it's supposed to mean.

You don't have to bring in an entire heavy dependency just for some slightly better assertion methods.

I don't see this as a major problem tbh - but yeah - it'd be nicer if the standard golang lib had some extra sugar on tests.

[lox]

Actually doesn't tell a great story about what it's testing.

Yeah I think it ends up being fairly subjective, I find it "information dense" and in this case it makes it easier to get the whole story on the one screen into my head, e.g which hooks should fire in which circumstances. The alternative would be lots and lots of lines that told a good story about individual hook tests, but not a story about the whole picture. 🤷🏼‍♂️ How would you have tackled that test case?

At any rate, I think some changes could be made to these tables to make them more readable, and I'll think more critically about whether to use table-based tests.

What did you think of the changes I made?

[keithpitt]

Yeah I think it ends up being fairly subjective

Totally, different people parse stuff differently. I think I'm more of a "tell a story with code" type person (which is why I'm very comment heavy).

Maybe I would have done something like:

func TestHookRunsAfterEnvironmentFailing(t *testing.T) {

  tester.ExpectGlobalHook("environment").
    Once().
    AndWriteToStderr("Blargh\n").
    AndExitWith(1)

  assetBootstrapResults(t,  &BootstrapResults{
    globalPreExitHookExecuted: true,
    checkoutHookExecuted: false
    artifactHookExecuted: false
  })
}

You may be able to read between the lines there, but I'd probably do something like that. Have some testing gear that runs the bootstrap and returns some "facts" about what the bootstrap did. Here the facts are returned as booleans. There could be other facts like:

  assetBootstrapResults(t,  &BootstrapResults{
    globalPreExitHookStatus: -1,
    globalPreExitHookStdout: "Blarghh!"
  })

Or something. The code examples above are totally wrong syntax wise, but hopefully you get the idea! (That's the sort of thing I'd probably aim for).

[keithpitt]

But the changes in this PR are good and you should 🚀

[lox]

I dig that example, I'll have a go at it! Good talk!