TEST: Add golden-file tests for provider record conversion

documentation/SUMMARY.md

@@ -222,6 +222,7 @@
 * [Writing new DNS providers](advanced-features/writing-providers.md)
 * [Creating new DNS Resource Types (rtypes)](advanced-features/adding-new-rtypes.md)
 * [Integration Tests](advanced-features/integration-tests.md)
+* [Provider conversion tests](developer-info/provider-conversion-tests.md)
 * [Test a branch](advanced-features/test-a-branch.md)
 * [Unit Testing DNS Data](advanced-features/unittests.md)
 * [Bug Triage Process](advanced-features/bug-triage.md)

documentation/developer-info/provider-conversion-tests.md

@@ -0,0 +1,192 @@
+# Provider conversion tests
+
+Most providers convert between their API's native record format and
+`models.RecordConfig`. Those conversion functions are only exercised by the
+integration tests, which need credentials and a test zone. When a provider's
+author becomes unavailable, nobody can run them.
+
+`pkg/providergolden` replays recorded data through those functions and compares
+the result with a golden file. The recorded data is checked in, so the tests run
+in `go test ./...` with no credentials and no network.
+
+Providers are enrolled one at a time. A provider with no recorded data is
+skipped, never failed.
+
+## Enrolling a provider
+
+### 1. Record the data
+
+The integration tests already drive every conversion a provider has, so the
+easiest way to collect the data is to record an integration run. Add `-record`
+to the command you normally use:
+
+```shell
+go test -run TestDNSProviders -timeout 1h -failfast -v ./integrationTest \
+  -args -verbose -profile CLOUDFLAREAPI -record
+```
+
+The recording goes to `providers/<package>/testdata`, the testdata directory of
+the package the provider under test is implemented in, so `CLOUDFLAREAPI` writes
+to `providers/cloudflare/testdata`. Use `-recorddir` to write somewhere else. A
+relative `-recorddir` is relative to the top of the repository:
+
+```shell
+go test -run TestDNSProviders -timeout 1h -failfast -v ./integrationTest \
+  -args -verbose -profile CLOUDFLAREAPI -recorddir providers/cloudflare/testdata
+```
+
+Either way two files are written, named after the provider's package:
+
+- `cloudflare.records` — every record the tests asked the provider to store,
+  which is what a `CheckToNative` function is given.
+- `cloudflare.json` — the native record each returned record came from, read
+  from `RecordConfig.Original`. That is what a `CheckToRC` function is given. It
+  is written only for providers that fill `Original` in.
+
+Those are the names the tests read, so there is nothing to rename. One recording
+feeds every test the provider has: a provider with two `CheckToNative` functions
+replays the same `.records` file through both. Read the files before committing
+them, though: they contain whatever your zone contained during the run.
+
+{% hint style="warning" %}
+The domain the test passes to `CheckToRC` and `CheckToNative` has to be the zone
+the data was recorded against, and nothing enforces it. `providergolden.Domain`
+takes it from the same `<PROVIDER>_DOMAIN` variable the integration tests use, so
+a recording and the test that replays it agree as long as that variable holds the
+zone the data came from. Where they disagree, native records carry labels as the
+API returned them, so a fixture recorded against `realzone.net` replayed by a
+test that says `example.com` produces a golden whose labels are still fully
+qualified:
+
+```
+www.realzone.net 300 IN A 192.0.2.1
+realzone.net 3600 IN MX 10 mail.example.org.
+```
+
+`LabelFromFQDNNoDot` and its siblings print `ERROR: ... called WRONG` when this
+happens, but they return the name lowercased rather than shortened and the test
+passes, so `-update` writes that golden and it becomes the baseline. Set the
+domain before recording the golden, not after.
+
+A golden recorded against a zone other than `example.com` only matches while
+`<PROVIDER>_DOMAIN` still names that zone, so it does not match in a checkout
+that does not set it. Record from `example.com` when the golden is to be
+committed.
+{% endhint %}
+
+{% hint style="warning" %}
+`Original` is recorded as it stands once the provider has built the zone, which
+is not always what the API sent. A converter that canonicalizes a value in place
+before storing the record in `Original` records a native that already carries
+the canonicalization, and a golden replayed from it never exercises the line
+that applies it. Check the recorded natives against the API's own responses when
+a provider's converter writes to the record it was given.
+{% endhint %}
+
+Without `-record` nothing is collected, no file is written and the provider is
+not wrapped, so a normal run is unchanged. Under `-record` the provider is
+wrapped in a `models.DNSProvider`, which is all the integration tests ask of it
+today. A test that type-asserts a provider to an optional interface such as
+`ZoneCreator` would need the wrapper to forward that interface too.
+
+Data can also be collected by hand. It goes in `providers/<package>/testdata`
+under the package's own name, `providers/netlify/testdata/netlify.json` and
+`providers/netlify/testdata/netlify.records`. The `.json` file is a JSON array
+of the native records your provider's API returns:
+
+```json
+[
+  {
+    "content": "192.0.2.1",
+    "id": 42,
+    "name": "www.example.com",
+    "ttl": 3600,
+    "type": "A"
+  }
+]
+```
+
+{% hint style="warning" %}
+Record from a throwaway zone. Native records carry labels, addresses, and record
+and zone IDs verbatim, which for a LAN appliance means your internal DNS ends up
+in a public repository.
+{% endhint %}
+
+### 2. Add the test
+
+The test adapts your conversion function to a uniform signature. That adapter is
+the only code you write:
+
+```go
+var testDomain = providergolden.Domain("NETLIFY")
+
+func TestToRecordConfigGolden(t *testing.T) {
+	providergolden.CheckToRC(t, "netlify_torecordconfig", testDomain,
+		func(dc *models.DomainConfig, native dnsRecord) ([]*models.RecordConfig, error) {
+			rc, err := toRecordConfig(dc, &native)
+			return []*models.RecordConfig{rc}, err
+		})
+}
+```
+
+The name is the golden file's, `netlify_torecordconfig.golden`, so name it after
+the function under test. The recorded input is `netlify.json`, found from the
+directory the test runs in rather than from that name, so several tests can
+replay one recording.
+
+`providergolden.Domain` returns `$NETLIFY_DOMAIN`, or `example.com` when that
+is unset, so the same test replays a recording of your own zone and the
+committed fixtures.
+
+Use `CheckToNative` for the other direction: `toNative`, `toReq`,
+`recordToCreateRequest`, or whatever your provider calls it. Its input is a list
+of DNS records rather than native records, so it reads a `.records` file:
+
+```go
+func TestToReqGolden(t *testing.T) {
+	providergolden.CheckToNative(t, "netlify_toreq", testDomain,
+		func(rc *models.RecordConfig) (*dnsRecordCreate, error) {
+			return toReq(rc), nil
+		})
+}
+```
+
+The `.golden` file produced by `CheckToRC` is a valid `.records` file, so the
+usual way to write one is to copy it and add whatever else you want covered.
+
+### 3. Generate the golden files
+
+```shell
+go test ./providers/netlify/ -update
+```
+
+Read the generated files before committing them. `-update` records whatever the
+code does today, including bugs.
+
+## The golden format
+
+One line per record: the label, TTL, class, type and RDATA, followed by the
+record's metadata when it has any:
+
+```
+www 300 IN A 192.0.2.1
+@ 3600 IN MX 10 mail.example.com.
+fwd 0 IN URL https://example.net/landing ; includePath="no" type="temporary" wildcard="no"
+```
+
+`CheckToNative` writes its golden as JSON, since a native record has no
+zonefile representation.
+
+## Updating
+
+Run `go test ./providers/<provider>/ -update` after an intentional change and
+review the diff. `-update` rewrites only the golden files: the recorded inputs
+are never touched, so updating a golden never needs an API key.
+
+## What these tests do and do not catch
+
+They pin a provider's conversion functions against their own past behaviour, so
+they catch a refactor that changes what a provider sends or understands. They do
+not catch a conversion that has been wrong since the day it was written, and
+they say nothing about the correction and apply path. That is still worth having
+for a provider whose integration tests nobody can run.

integrationTest/helpers_test.go

@@ -10,19 +10,26 @@ import (
 	"testing"
 
 	"github.com/DNSControl/dnscontrol/v5/pkg/credsfile"
+	"github.com/DNSControl/dnscontrol/v5/pkg/providergolden"
 	"github.com/DNSControl/dnscontrol/v5/pkg/providers"
 	"github.com/DNSControl/dnscontrol/v5/providers/cloudflare"
 )
 
 var (
 	providerFlag         = flag.String("provider", "", "Provider to run (if empty, deduced from -profile)")
 	profileFlag          = flag.String("profile", "", "Entry in profiles.json to use (if empty, copied from -provider)")
+	recordFlag           = flag.Bool("record", false, "Write the record conversion inputs seen during the run to the provider's testdata directory")
+	recordDirFlag        = flag.String("recorddir", "", "Directory to record into, and implies -record (default: the provider's testdata directory)")
 	enableCFWorkers      = flag.Bool("cfworkers", true, "enable CF worker tests (default false)")
 	enableCFRedirectMode = flag.Bool("cfredirect", true, "enable CF SingleRedirect tests (default false)")
 	enableCFFlatten      = flag.Bool("cfflatten", false, "enable CF CNAME flattening tests (requires paid plan, default false)")
 	enableCFTags         = flag.Bool("cftags", false, "enable CF tag tests (requires paid plan, default false)")
 )
 
+// recorder accumulates the conversion inputs of every provider call made by
+// this run, and is written out when -record is given.
+var recorder = providergolden.NewRecorder()
+
 func init() {
 	testing.Init()
 
@@ -38,6 +45,10 @@ func panicOnErr(err error) {
 }
 
 func getProvider(t *testing.T) (providers.DNSServiceProvider, string, map[string]string) {
+	if flag.NArg() != 0 {
+		t.Fatalf("unexpected argument %q; the recording directory is set with -recorddir", flag.Arg(0))
+	}
+
 	if *providerFlag == "" && *profileFlag == "" {
 		t.Log("No -provider or -profile specified")
 		return nil, "", nil
@@ -124,5 +135,39 @@ func getProvider(t *testing.T) (providers.DNSServiceProvider, string, map[string
 		}
 	}
 
+	if *recordFlag || *recordDirFlag != "" {
+		dir, err := recordingDir(provider)
+		if err != nil {
+			t.Fatal(err)
+		}
+		name, err := providergolden.ProviderName(provider)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Cleanup(func() { writeRecording(t, dir, name) })
+		return providergolden.Record(provider, recorder), cfg["domain"], cfg
+	}
+
 	return provider, cfg["domain"], cfg
 }
+
+// recordingDir is where a recording of p is written: -recorddir when it is
+// given, and otherwise p's own testdata directory.
+func recordingDir(p providers.DNSServiceProvider) (string, error) {
+	if *recordDirFlag != "" {
+		return providergolden.ResolveDir(*recordDirFlag)
+	}
+	return providergolden.TestdataDir(p)
+}
+
+// writeRecording writes everything recorded so far to dir, named after the
+// provider under test.
+func writeRecording(t *testing.T, dir, name string) {
+	written, err := recorder.WriteTo(dir, name)
+	for _, path := range written {
+		t.Logf("Recorded %s", path)
+	}
+	if err != nil {
+		t.Error(err)
+	}
+}

integrationTest/recording_test.go

@@ -0,0 +1,49 @@
+package main
+
+// Test where a recording is written.
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/DNSControl/dnscontrol/v5/pkg/providergolden"
+	"github.com/DNSControl/dnscontrol/v5/pkg/providers"
+)
+
+func TestARecordingIsNamedAfterTheProvidersPackageNotItsType(t *testing.T) {
+	provider, err := providers.CreateDNSProvider("GANDI_V5", map[string]string{"token": "not used"}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	name, err := providergolden.ProviderName(provider)
+	if err != nil {
+		t.Fatalf("ProviderName() error: %v", err)
+	}
+	if name != "gandiv5" {
+		t.Errorf("ProviderName() = %q, want %q", name, "gandiv5")
+	}
+}
+
+func TestRecordingDirResolvesRecordDirFromTheModuleRoot(t *testing.T) {
+	rel := filepath.Join("providers", "bind", "testdata")
+
+	old := *recordDirFlag
+	*recordDirFlag = rel
+	defer func() { *recordDirFlag = old }()
+
+	dir, err := recordingDir(nil)
+	if err != nil {
+		t.Fatalf("recordingDir() error: %v", err)
+	}
+	if !filepath.IsAbs(dir) {
+		t.Fatalf("recordingDir() = %q, want an absolute path", dir)
+	}
+
+	root := strings.TrimSuffix(dir, string(filepath.Separator)+rel)
+	if _, err := os.Stat(filepath.Join(root, "go.mod")); err != nil {
+		t.Errorf("recordingDir() = %q, want a path under the directory holding go.mod: %v", dir, err)
+	}
+}