add buildkite upload

[finn-block]

and re-arranged some stuff:

• split out all report renders to their own file
• update result struct to track per-test runtime
• disable failing web5-kt test
• use web5-js test from web5-js repo, drop web5-js test from this repo
• renamed the go package to reflect the current repo name

[andresuribe87]

Should there be tests for the template producing pieces of the scaffolding?

[andresuribe87]

Would it make sense to check <200?

[finn-block]

I can't find any documentation on the expected error codes from buildkite for this endpoint, but > 299 will catch any normal error code. < 200 is "informational" (according to MDN)

[decentralgabe]

we have a helper func in other libs that is like

func is200Status(code int) bool {
  return code / 200 != 2 
}

[andresuribe87]

I noticed the pattern of logging errors liberally, which results in logging the same error multiple times. This isn't a big deal, but wanted to share some principles I've found to produce very readable code with excellent logging of errors:

• Return wrapped errors when calling a function that's exported from another package which returns an error, or when you want to add additional context.
• Log at the root level where errors are bubbled up to.

Let me elaborate with an example.

func runCmd() {
  if err := buildkiteUpload(); err != nil {
    slog.Error("uploading buildkite", err)
  }
}

func buildkiteUpload() error {
  if err := addFormValue("foo"); err != nil {
    return errors.Wrap(err, "adding form value foo")
  }
  if err := addFormValue("bar"); err != nil {
    return errors.Wrap(err, "adding form value bar")
  }
  return nil
}

func addFormValue() error {
   if err := DoExternalPackageCallThatMayError(); err != nil {
    return errors.Wrap(err, "doing external package call")
  }
  return nil
}

Notice how logging is only done at the top level function. When reading the log, the causality chain will be readable. I.e. the error will be logged as adding form value foo : doing external package call : memory failed to allocate. Additionally, the stack trace will also be logged, so you know exactly where the problem was.

[finn-block]

I'm certainly familiar with that pattern, but personally prefer the the context messages related to the error to be logged on separate lines. I find it more readable. If the exact same message is getting repeated multiple times, that's an issue, but this is just adding context that may or may not be relevant to help understand where the error originated. I find the pattern of having an increasingly long error message with all the context on a single line to be confusing and hard to read.

If people have strong opinions, I'm happy to change this, but if it's my code and I'm going to have to debug it, I would prefer the approach i've adopted.

[andresuribe87]

I've already expressed my opinion :)
No big deal, I think it's an implementation detail.

[decentralgabe]

nit: let's favor ssh checkout over https

[finn-block]

this results in a failure (ci output):

[andresuribe87]

@finn-tbd just wanted to make sure you didn't miss #68 (review)

tl;dr; should all the code in here be tested? Apologies if it's being tested already and I missed it.

Otherwise this lgtm

[finn-block]

@andresuribe87 this is all tested, most of it in the branch's CI:

• junit (new template) is rendered and sent to buildkite in CI because the token is present (results here)
• md is rendered and written to the github summary (link to rendered md from the most recent pipeline)
• text is rendered and displayed in the Github Actions text output (bottom of "self test" job from this pipeline)
• html template i tested locally

edit: unless you're talking about more automated correctness verification like unit tests or something. I don't feel those are necessary right now, especially with the desire to increase our velocity, but if others disagree I'm open to looking at ways to unit test the templates.

[andresuribe87]

edit: unless you're talking about more automated correctness verification like unit tests or something. I don't feel those are necessary right now, especially with the desire to increase our velocity, but if others disagree I'm open to looking at ways to unit test the templates.

I was talking about those automated tests. I'd expect things to be really easy to test if you already have samples. I would argue not having them decreases our velocity in the medium term, as modifications to anything in the upload pipeline will be hard to verify.