Make error handling more testable

[RothAndrew]

I'm looking at writing a unit test for helping with #45, but because of the use of logContext.Fatal() in the codebase a unit test passes when it shouldn't.

I propose replacing all logContext.Fatal() commands with logContext.Error() and return the error type wherever logContext.Fatal() was being used.

A simple example of this would be to change this:

func Foo() {
  err := doSomethingThatDoesntWork()
  if err != nil {
    logContext.Fatal("some error")
  }
}

to this:

func Foo() error {
  err := doSomethingThatDoesntWork()
  if err != nil {
    e := "some error"
    logContext.Error(e)
    return fmt.Errorf(e)
  }
  return nil
}

[RothAndrew]

For now I'm using this: https://stackoverflow.com/a/54247636/2946639

but from the (admittedly not extensive) research I've done on this subject I believe this is still worth changing.

[jeff-mccoy]

I'd actually like to wait before we introduce unit testing again, would like more discussion/research and am a big fan of BDD if we do add unit tests back. I think the priority should be finishing the Ephemeral E2E for now as they are more robust and would have caught this as well.

[jeff-mccoy]

Other comment, yes I've written libs that relied upon error returning flow, but the decision to use Fatal was intentional as just about everything in Zarf should be an absolute fail. It's true that not passing errors makes generic unit testing harder but I'd like to 1. Discuss how much we care about that as other libs (check out some of Rancher's tools) rely on E2E over unit as well, 2. We aren't just panicking, so we could easily mock that or wrap .Fatal() in another function.

[RothAndrew]

You'd use BDD for unit-level testing? That feels weird to me. I'm used to BDD being used for higher level tests like E2E or maybe integration.

[RothAndrew]

I'm a big proponent of the majority of an application's test suite being unit tests. Too many E2E tests makes for a brittle test suite since they tend to touch things everywhere.

That being said, I'm fine with deferring to your opinion here since you write the vast majority of the codebase and if it works for you then who am I to diss it 😄

[RothAndrew]

Regardless of which type of testing IMO we need to be able to do this level of debugging:

The Terratest work in the other PR would not give us this, since the flow is

1. Compile the binary and SCP it to the EC2 instance
2. Run shell commands
3. Assert shell commands

It's good as a high level smoke test but bad for debugging fiddly issues in individual methods

[jeff-mccoy]

FWIW, I actually do run the vscode debugger all the time without unit tests, just using the packager directly. For BDD unit tests, yes done quite a bit, here's what I'm looking at for zarf: https://github.com/onsi/ginkgo.

I have a pretty different (maybe jaded) philosphy on unit tests in general. I've seen far too many barely-useful tests that are mostly meaningless just to reach code coverage % or worse tests just to have tests that are actually bad tests. In my mind the unit tests are best in atomic scenarios, i.e. functional test strategies. But go libs have a heavy use of pointers / by-reference functions that makes this far less clean. The only real benefit to unit tests for me is the speed of feedback.

We don't have a database to deal with or anything like a web app would, so as long as zarf is fast, E2E doesn't have to be super slow and you can still debug E2E the same, we just need to refactor the E2E stuff a little bit so it can run local or remote.

All that being said, I'm not totally opposed to unit tests either and for clean functions it can make a lot of sense, but E2E tests are always the real tests because they test actually functionality. In the case of this example, we would ideally have just run the tiny-kafka example as an E2E test.

[RothAndrew]

Gotcha. I was thinking cucumber/gherkin style BDD vs the mocha/jest style that this looks like, which i agree would support unit-level testing just fine.

[jeff-mccoy]

Yeah I'm not that fancy 🤣