miscellaneous documentation fixes (#165)

Addresses items in #164 and some others.

* remove outdated comment

* add TESTING.md

* prefer otel spec envvar over OTEL_CLI

* add a section on adding tests

* write nice top-of-file comments and make them similar

* remove extra 'a'

* add a note about CSV parsing of attributes & headers

Brought to you by #153.

README.md

@@ -111,7 +111,7 @@ then config file, then environment variables.
 | --config             | OTEL_CLI_CONFIG_FILE          | config_file              | config.json    |
 | --verbose            | OTEL_CLI_VERBOSE              | verbose                  | false          |
 | --fail               | OTEL_CLI_FAIL                 | fail                     | false          |
-| --service            | OTEL_CLI_SERVICE_NAME         | service_name             | myapp          |
+| --service            | OTEL_SERVICE_NAME             | service_name             | myapp          |
 | --kind               | OTEL_CLI_TRACE_KIND           | span_kind                | server         |
 | --status-code        | OTEL_CLI_STATUS_CODE          | span_status_code         | error          |
 | --status-description | OTEL_CLI_STATUS_DESCRIPTION   | span_status_description  | cancelled      |
@@ -136,6 +136,19 @@ http endpoint, set the protocol with --protocol or the envvar.
    * `http://` and `https://` are assumed to be HTTP unless --protocol is set to `grpc`.
    * loopback addresses without an https:// prefix are assumed to be unencrypted
 
+### Header and Attribute formatting
+
+Headers and attributes allow for `key=value,k=v` style formatting. Internally both
+otel-cli and pflag use Go's `encoding/csv` to parse these values. Therefore, if you want
+to pass commas in a value, follow CSV quoting rules and quote the whole k=v pair.
+Double quotes need to be escaped so the shell doesn't interpolate them. Once that's done,
+embedding commas will work fine.
+
+```shell
+otel-cli span --attrs item1=value1,\"item2=value2,value3\",item3=value4
+otel-cli span --attrs 'item1=value1,"item2=value2,value3",item3=value4'
+```
+
 ## Easy local dev
 
 We want working on otel-cli to be easy, so we've provided a few different ways to get

TESTING.md

@@ -0,0 +1,80 @@
+# Testing otel-cli
+
+## Synopsis
+
+otel-cli's primary method of testing is functional, implemented in
+`main_test.go` and accompanying files. It sets up a server and runs an otel-cli
+build through a number of tests to verify that everything from environment
+variable passing to server TLS negotiation work as expected.
+
+## Unit Testing
+
+Do it. It doesn't have to be fancy, just exercise the code a little. It's more
+about all of us being able to iterate quickly than reaching total coverage.
+
+Most unit tests are in the `otelcli` package. The tests in the root of this
+project are not unit tests.
+
+## The otel-cli Test Harness
+
+When `go test` is run in the root of this project, it runs the available
+`./otel-cli` binary through a suite of tests, providing otel-cli with its
+endpoint information (via templates) and examining the payloads received on the
+server.
+
+The otel-cli test harness is more complex than otel-cli itself. Its goal is to
+be able to test that setting e.g. `OTEL_EXPORTER_OTLP_CLIENT_KEY` works all the
+way through to authenticating to a TLS server. The bugs are going to exist in
+the glue code, since that's mostly what otel-cli is. Each of Cobra,
+`encoding/json`, and opentelemetry-go are thorougly unit and battle tested. So,
+otel-cli tests a build in a functional test harness.
+
+Tests are defined in `data_for_test.go` in Go data structures. Suites are a
+group of Fixtures that go together. Mostly Suites are necessary for the
+backgrounding feature, to test e.g. `otel-cli span background`, and to organize
+tests by functionality, etc.. Fixtures configure everything for an otel-cli
+invocation, and what to expect back from it.
+
+The OTLP server functionality originally built for `otel-cli server tui` is
+re-used in the tests to run a server in a goroutine. It supports both gRPC and
+HTTP variants of OTLP, and can be configured with TLS. This allows otel-cli to
+connect to a server and send traces, which the harness then compares to
+expectations defined in the test Fixture.
+
+otel-cli has a special subcommand, `otel-cli status` that sends a span and
+reports back on otel-cli's internal state. The test harness detects status
+commands and can check data in it.
+
+`tls_for_test.go` implements an ephemeral certificate authority that is created
+and destroyed on each run. The rest of the test harness injects the CA and certs
+created into the tests, allowing for full-system testing.
+
+A Fixture can be configured to run "in the background". In this case, the harness
+will behave as if you ran the command `./otel-cli &` and let following fixtures
+run on top of it. This is mostly used to test `otel-cli span background`, which
+exists primarily to run as a backgrounded job in a shell script. When background
+jobs are in use, be careful with test names as they are used as a key to manage
+the process.
+
+## Adding Tests
+
+For a new area of functionality, you'll want to add a Suite. A Suite is mostly
+for organization of tests, but is also used to manage state when testing background
+jobs. A Fixture is made of two parts: an otel-cli command configuration, and a
+data structure of expected results. The harness presents otel-cli with the exact
+ARGV specified in `Config.CliArgs`, and a clean environment with only the envvars
+provided in the `Env` stringmap. The output from otel-cli is captured with stdout
+and stderr combined. This can be tested against as well.
+
+When otel-cli reads an environment variable it clears it, to prevent opentelemetry-go
+from double-processing it. This is why envvars set in `Config.Env` don't show up
+in `Results.Env`.
+
+It is often wise to pass `"--fail", "--verbose"` to CliArgs for debugging and it's
+fine to leave them on permanently. Without them otel-cli will be silent about
+failures and you'll get a confusing test output.
+
+Most of the time it's best to copy an existing Suite or Fixture and adjust it to
+the case you're trying to test. Please try to clean out any unneeded config when
+you do this so the tests are easy to understand. It's not bad to to test a little
+extra surface area, just try to keep things readable.

data_for_test.go

@@ -1,9 +1,11 @@
 package main_test
 
-// Data structures and data for functional testing of otel-cli.
-
+// This file implements data structures and data for functional testing of
+// otel-cli.
+//
+// See: TESTING.md
+//
 // TODO: Results.SpanData could become a struct now
-// TODO: add instructions for adding more tests
 
 import (
 	"regexp"

main_test.go

@@ -1,8 +1,10 @@
 package main_test
 
-// implements the data-driven tests of otel-cli using data in data_for_test.go
-
-// TODO: stop using fixture.Name to track foreground/background
+// This file implements the data-driven test harness for otel-cli. It executes
+// tests defined in data_for_test.go, and uses the CA implementation in
+// tls_for_test.go.
+//
+// see TESTING.md for details
 
 import (
 	"crypto/tls"

tls_for_test.go

@@ -119,10 +119,6 @@ func generateTLSData(t *testing.T) tlsHelpers {
 		t.Fatalf("error generating server cert pair: %s", err)
 	}
 
-	// In theory, the server shouldn't need this CA in the RootCAs pool to accept client
-	// connections. Without it, grpc refuses the client connection with invalid CA.
-	// No amount of client config changes would work. The opentelemetry collector also sets
-	// RootCAs by default so it seems safe to copy that behavior here.
 	out.serverTLSConf = &tls.Config{
 		ClientCAs:    out.certpool,
 		Certificates: []tls.Certificate{serverCertPair},