From e173ed3bfb62492afbad90371c1c2ca7819f33a0 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 24 Aug 2026 16:39:05 +0200 Subject: [PATCH 1/5] feat(provider-tck): emit a machine-readable conformance report Setting PROVIDER_TCK_REPORT_DIR makes each suite write its run to /.json against the report schema in the specification repository (open-feature/spec#425, part of open-feature/spec#424). Go is the language that needs this first. godog counts a capability-gated skip in its passed tally, so a run that skipped five of twenty-nine scenarios prints "29 scenarios (29 passed)". Appendix F is unambiguous that a scenario skipped for an undeclared capability is reported as skipped with the reason and never as passed, and the harness does say so in a separate log line -- but the headline number still says something false, and the number is what gets read. pytest and jest-cucumber report skips correctly, so this is the runner's property rather than the suite's design. The report does not fix godog's summary. It makes the summary stop mattering, by recording every scenario's outcome individually so a consumer can check the rule instead of trusting the runner to have applied it. The same run now reports twenty-four passed and five not-declared, each with its reason. Identity comes from revision.go, generated by sync_assets.go beside the embedded artifacts. Generating both in one command is what keeps them honest: the CI check regenerates and fails on any difference, so a revision disagreeing with the artifacts beside it cannot be committed. The check is widened to cover the generated file, which it would otherwise have missed. Both the commit and the tree hash are recorded, the tree because it identifies the artifacts alone -- unchanged by unrelated edits elsewhere in the specification, so two runs of identical artifacts agree even when pinned to different commits, and checkable because `git rev-parse :specification/assets/provider-tck` reproduces it. Two smaller decisions. The provider is identified by the name it reports through its own metadata, with Config.Name recorded as the configuration, because Config.Name is chosen to read well in a failure message -- "flagd-rpc" -- and a provider with two materially different modes produces two reports that are not interchangeable. And how the backend was driven is read through an optional interface rather than a new BackendControl method, so that adding it breaks no existing implementation and a control that does not implement it simply omits the field. Emission is opt-in through the environment rather than through Config so that producing a report is a property of the run and not of the code: CI sets it, a local run does not, and no adopter changes a line to publish one. The tests assert the property that motivated the work -- that no scenario the capability gate stopped is ever reported as passed, and that every scenario is accounted for exactly once, since a report that silently omitted what it skipped would satisfy the first half while still misleading a reader. That test earned its place immediately: it caught this emitter recording every skipped scenario twice, the second time as passed, because godog does not deliver the before hook's ErrSkip to the after hook. Signed-off-by: Simon Schrottner --- Makefile | 2 +- tools/provider-tck/README.md | 57 ++++ tools/provider-tck/pkg/tck/capability.go | 10 +- tools/provider-tck/pkg/tck/inprocess.go | 7 + tools/provider-tck/pkg/tck/report.go | 336 +++++++++++++++++++ tools/provider-tck/pkg/tck/report_test.go | 195 +++++++++++ tools/provider-tck/pkg/tck/revision.go | 15 + tools/provider-tck/pkg/tck/run.go | 110 +++++- tools/provider-tck/pkg/tck/state.go | 6 + tools/provider-tck/pkg/tck/steps_provider.go | 1 + tools/provider-tck/sync_assets.go | 80 ++++- 11 files changed, 808 insertions(+), 11 deletions(-) create mode 100644 tools/provider-tck/pkg/tck/report.go create mode 100644 tools/provider-tck/pkg/tck/report_test.go create mode 100644 tools/provider-tck/pkg/tck/revision.go diff --git a/Makefile b/Makefile index fa1455997..1886d40a0 100644 --- a/Makefile +++ b/Makefile @@ -21,7 +21,7 @@ provider-tck-assets: cd tools/provider-tck && go run ./sync_assets.go provider-tck-assets-check: provider-tck-assets - git diff --exit-code -- tools/provider-tck/pkg/tck/assets + git diff --exit-code -- tools/provider-tck/pkg/tck/assets tools/provider-tck/pkg/tck/revision.go test: go list -f '{{.Dir}}/...' -m | xargs -I{} go test -v {} diff --git a/tools/provider-tck/README.md b/tools/provider-tck/README.md index 526f1922c..8018d96ab 100644 --- a/tools/provider-tck/README.md +++ b/tools/provider-tck/README.md @@ -204,6 +204,62 @@ provider in a domain replaces and shuts down the previous one; a fresh domain pe leave every provider of the suite registered and running, which for a provider holding a network connection means leaking one connection per scenario. +## Conformance reports + +Set `PROVIDER_TCK_REPORT_DIR` and each suite writes a machine-readable report of its run to +`/.json`, conforming to the [report schema][report-schema] in the specification. + +```console +$ PROVIDER_TCK_REPORT_DIR=./reports go test ./... +$ jq '.scenarios | group_by(.outcome) | map({(.[0].outcome): length}) | add' reports/in-memory.json +{ + "passed": 24, + "not-declared": 5 +} +``` + +It is an environment variable rather than a `Config` field so that emitting a report is a property +of the run and not of the code: CI sets it, a developer running the suite locally does not, and no +adopter changes a line to publish one. Unset means no report, which is not an error. Several suites +in one test binary each write their own file, so flagd's two resolvers do not collide. + +### Why this exists in Go before the other languages + +Because Go is the language that needs it most. godog counts a capability-gated skip in its **passed** +tally: + +``` +29 scenarios (29 passed) +``` + +Five of those twenty-nine did not run. Appendix F is unambiguous that a scenario skipped for an +undeclared capability is reported as skipped with the reason and *never* as passed, and the harness +does say so in a separate log line — but the headline number still says something false, and a +number is what gets read. pytest and jest-cucumber both report skips correctly, so this is a property +of the runner rather than of the suite's design. + +The report does not fix godog's summary. It makes the summary stop mattering, by recording the +outcome of every scenario individually so that a consumer can check the rule instead of trusting the +runner to have applied it. `reports/in-memory.json` above accounts for all twenty-nine scenarios and +calls five of them `not-declared`, each with the reason. + +### What identifies a report + +`tck.specRevision` and `tck.assetsTree` come from [`revision.go`](./pkg/tck/revision.go), which +`sync_assets.go` generates from the submodule alongside the embedded artifacts. Generating both in +the same command is what keeps them honest: `make provider-tck-assets-check` regenerates and fails on +any difference, so a revision that disagrees with the artifacts beside it cannot be committed. + +The tree hash is carried as well as the commit because it identifies the artifacts alone. It is +unchanged by unrelated edits elsewhere in the specification, so two runs that executed identical +artifacts report the same value even when pinned to different commits — and it is checkable, since +`git rev-parse :specification/assets/provider-tck` must reproduce it. + +`provider.name` is what the provider reports through its own metadata, not `Config.Name`. +`Config.Name` is chosen to read well in a failure message — `flagd-rpc` — which makes it the +*configuration*, and it is reported as such. One provider with two materially different modes +produces two reports that are not interchangeable. + ## The self-tests Three suites run against providers from the SDK itself. They need no Docker and finish in @@ -264,5 +320,6 @@ the SDK's provider rather than reimplementing it — every resolution decision i [appendix-b]: https://github.com/open-feature/spec/blob/main/specification/appendix-b-gherkin-suites.md [appendix-f]: https://github.com/open-feature/spec/blob/main/specification/appendix-f-provider-conformance.md [control-api]: https://github.com/open-feature/spec/blob/main/specification/assets/provider-tck/openapi/control-api.yaml +[report-schema]: https://github.com/open-feature/spec/blob/main/specification/assets/provider-tck/report/conformance-report.schema.json [spec]: https://github.com/open-feature/spec [tracking]: https://github.com/open-feature/spec/issues/417 diff --git a/tools/provider-tck/pkg/tck/capability.go b/tools/provider-tck/pkg/tck/capability.go index 46829a8f2..026a5d997 100644 --- a/tools/provider-tck/pkg/tck/capability.go +++ b/tools/provider-tck/pkg/tck/capability.go @@ -139,9 +139,13 @@ func (c Capability) Tag() string { return string(c) } // String implements fmt.Stringer. func (c Capability) String() string { return string(c) } -// capabilityForTag maps a Gherkin tag onto the capability it gates, reporting +// CapabilityForTag maps a Gherkin tag onto the capability it gates, reporting // whether the tag gates anything at all. -func capabilityForTag(tag string) (Capability, bool) { +// +// Exported because a conformance report is read by things outside this package: +// deciding whether a scenario was skipped legitimately means knowing which of +// its tags gate a capability and which are merely organisational. +func CapabilityForTag(tag string) (Capability, bool) { for _, c := range allCapabilities { if string(c) == tag { return c, true @@ -156,7 +160,7 @@ type capabilitySet map[Capability]struct{} func newCapabilitySet(caps []Capability) (capabilitySet, error) { set := make(capabilitySet, len(caps)) for _, c := range caps { - if _, known := capabilityForTag(string(c)); !known { + if _, known := CapabilityForTag(string(c)); !known { return nil, fmt.Errorf( "unknown capability %q: capabilities are the constants declared in this package, one of %s", c, formatCapabilities(allCapabilities)) diff --git a/tools/provider-tck/pkg/tck/inprocess.go b/tools/provider-tck/pkg/tck/inprocess.go index 49cb968fd..0fe14d75c 100644 --- a/tools/provider-tck/pkg/tck/inprocess.go +++ b/tools/provider-tck/pkg/tck/inprocess.go @@ -234,3 +234,10 @@ func (c *InProcessControl) ChangeFlag(context.Context) error { return c.current.UpdateFlag(ChangingFlagKey, changingFlag(c.changingVariant)) } + +// ControlAPI reports how this backend was driven. +// +// "in-process" is the narrow allowance the schema makes for a provider with no +// backend. A report claiming it for a provider that has one should be treated +// with suspicion, which is precisely why it is recorded rather than assumed. +func (c *InProcessControl) ControlAPI() string { return "in-process" } diff --git a/tools/provider-tck/pkg/tck/report.go b/tools/provider-tck/pkg/tck/report.go new file mode 100644 index 000000000..17e4bcfc0 --- /dev/null +++ b/tools/provider-tck/pkg/tck/report.go @@ -0,0 +1,336 @@ +package tck + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "runtime/debug" + "sort" + "strings" + "time" +) + +// ReportDirEnv names the directory a conformance report is written to. +// +// It is an environment variable rather than a Config field so that emitting a +// report is a property of the run and not of the code: CI sets it, a developer +// running the suite locally does not, and no adopter has to change a line to +// publish one. A suite writes /.json, so several suites in one test +// binary — flagd's RPC and in-process resolvers, say — each produce their own +// file without colliding. +// +// Unset means no report, which is the default and is not an error. +const ReportDirEnv = "PROVIDER_TCK_REPORT_DIR" + +// reportSchemaVersion is the major version of the report schema this emitter +// produces. See specification/assets/provider-tck/report/. +const reportSchemaVersion = "1" + +// Outcome is the result of one scenario, or of one capability. +// +// There are four rather than two because "did not run" is not one thing. +// A capability the provider chose not to declare is a different statement from +// one the language makes impossible — @strict-numeric-typing cannot hold in a +// language with no integer type — and reporting both as "not declared" would +// show a whole language as missing something none of its providers can have. +type Outcome string + +const ( + OutcomePassed Outcome = "passed" + OutcomeFailed Outcome = "failed" + OutcomeNotDeclared Outcome = "not-declared" + OutcomeNotApplicable Outcome = "not-applicable" +) + +// Report is one run of the suite against one provider in one configuration. +// +// The field names and shape are fixed by the schema in the specification +// repository; this type is deliberately a transcription of it rather than a +// convenient Go representation, because the point of the format is that four +// languages emit the same thing. +type Report struct { + SchemaVersion string `json:"schemaVersion"` + Provider ReportProvider `json:"provider"` + SDK ReportSDK `json:"sdk"` + TCK ReportTCK `json:"tck"` + Backend *ReportBackend `json:"backend,omitempty"` + Capabilities map[string]ReportCapability `json:"capabilities"` + Scenarios []ReportScenario `json:"scenarios"` +} + +type ReportProvider struct { + Name string `json:"name"` + Version string `json:"version,omitempty"` + Language string `json:"language"` + Configuration string `json:"configuration,omitempty"` +} + +type ReportSDK struct { + Name string `json:"name"` + Version string `json:"version"` +} + +type ReportTCK struct { + Implementation string `json:"implementation"` + Version string `json:"version"` + SpecRevision string `json:"specRevision"` + SpecRelease string `json:"specRelease,omitempty"` + AssetsTree string `json:"assetsTree,omitempty"` +} + +type ReportBackend struct { + Description string `json:"description,omitempty"` + ControlAPI string `json:"controlApi,omitempty"` +} + +type ReportCapability struct { + State Outcome `json:"state"` + Reason string `json:"reason,omitempty"` +} + +type ReportScenario struct { + Feature string `json:"feature"` + Name string `json:"name"` + Tags []string `json:"tags,omitempty"` + Outcome Outcome `json:"outcome"` + Reason string `json:"reason,omitempty"` + DurationMs float64 `json:"durationMs,omitempty"` +} + +// scenarioRecord is what the runner accumulates as scenarios execute. +type scenarioRecord struct { + feature string + name string + tags []string + outcome Outcome + reason string + duration time.Duration +} + +// buildReport assembles the report from what the run observed. +// +// The per-scenario list is the load-bearing part. Appendix F requires that a +// scenario skipped for an undeclared capability is never reported as passed, +// and godog's own summary does exactly that — it counts capability skips in its +// passed tally, so the headline number says something false. Emitting the +// outcome of every scenario individually makes the rule checkable by a consumer +// instead of dependent on each runner's summary being trustworthy. +func (r *runner) buildReport() Report { + providerName := r.observedProviderName() + r.mu.Lock() + records := make([]scenarioRecord, len(r.records)) + copy(records, r.records) + r.mu.Unlock() + + sort.Slice(records, func(i, j int) bool { + if records[i].feature != records[j].feature { + return records[i].feature < records[j].feature + } + return records[i].name < records[j].name + }) + + scenarios := make([]ReportScenario, 0, len(records)) + // failedCapabilities tracks which declared capabilities had a scenario fail, + // so a capability is only reported as passed when everything gating on it + // actually passed. + failedCapabilities := map[Capability]bool{} + + for _, rec := range records { + scenarios = append(scenarios, ReportScenario{ + Feature: rec.feature, + Name: rec.name, + Tags: rec.tags, + Outcome: rec.outcome, + Reason: rec.reason, + DurationMs: float64(rec.duration.Microseconds()) / 1000.0, + }) + if rec.outcome != OutcomeFailed { + continue + } + for _, tag := range rec.tags { + if capability, gates := CapabilityForTag(tag); gates { + failedCapabilities[capability] = true + } + } + } + + capabilities := map[string]ReportCapability{} + for _, capability := range AllCapabilities() { + switch { + case !r.caps.has(capability): + capabilities[capability.Tag()] = ReportCapability{ + State: OutcomeNotDeclared, + Reason: fmt.Sprintf( + "not declared by this provider's configuration; the %s scenarios were skipped and did not contribute to this result", + capability.Tag()), + } + case failedCapabilities[capability]: + capabilities[capability.Tag()] = ReportCapability{State: OutcomeFailed} + default: + capabilities[capability.Tag()] = ReportCapability{State: OutcomePassed} + } + } + + return Report{ + SchemaVersion: reportSchemaVersion, + Provider: ReportProvider{ + Name: providerName, + Language: "go", + Configuration: r.cfg.Name, + }, + SDK: ReportSDK{Name: goSDKModule, Version: sdkVersion()}, + TCK: ReportTCK{ + Implementation: tckImplementation, + Version: tckVersion(), + SpecRevision: SpecRevision, + AssetsTree: AssetsTree, + }, + Backend: &ReportBackend{ + Description: r.cfg.Control.Description(), + ControlAPI: controlAPIOf(r.cfg.Control), + }, + Capabilities: capabilities, + Scenarios: scenarios, + } +} + +// observedProviderName is what the provider called itself, falling back to the +// suite name when no scenario ever registered one -- which happens when every +// scenario was skipped, and is worth reporting as the suite name rather than as +// an empty string the schema would reject. +func (r *runner) observedProviderName() string { + r.mu.Lock() + defer r.mu.Unlock() + if r.providerName != "" { + return r.providerName + } + return r.cfg.Name +} + +// controlAPIReporter is implemented by a BackendControl that knows which kind +// of control the schema should record. +// +// It is an optional interface rather than a method on BackendControl because +// adding a method would break every existing implementation for the sake of one +// string, and a control that does not implement it simply omits the field. +type controlAPIReporter interface { + // ControlAPI reports "http" for the normative control API or "in-process" + // for the narrow allowance made for providers with no backend. + ControlAPI() string +} + +func controlAPIOf(control BackendControl) string { + if reporter, ok := control.(controlAPIReporter); ok { + return reporter.ControlAPI() + } + return "" +} + +const ( + goSDKModule = "github.com/open-feature/go-sdk" + tckImplementation = "go-sdk-contrib/tools/provider-tck" + tckModule = "github.com/open-feature/go-sdk-contrib/tools/provider-tck" +) + +// writeReport emits the report if ReportDirEnv is set. +// +// A failure to write is reported as a test failure rather than logged and +// ignored. CI that asked for a report and silently did not get one is how a +// publishing pipeline ends up serving a stale result forever. +func (r *runner) writeReport() { + dir := strings.TrimSpace(os.Getenv(ReportDirEnv)) + if dir == "" { + return + } + + report := r.buildReport() + + data, err := json.MarshalIndent(report, "", " ") + if err != nil { + r.t.Errorf("provider-tck [%s]: could not encode the conformance report: %v", r.cfg.Name, err) + return + } + data = append(data, '\n') + + if err := os.MkdirAll(dir, 0o755); err != nil { + r.t.Errorf("provider-tck [%s]: could not create the report directory %s: %v", r.cfg.Name, dir, err) + return + } + + path := filepath.Join(dir, reportFileName(r.cfg.Name)) + if err := os.WriteFile(path, data, 0o644); err != nil { + r.t.Errorf("provider-tck [%s]: could not write the conformance report to %s: %v", r.cfg.Name, path, err) + return + } + + r.t.Logf("provider-tck [%s]: conformance report written to %s", r.cfg.Name, path) +} + +// reportFileName turns a suite name into a filename. +// +// Suite names are chosen to read well in failure messages rather than to be +// path-safe, so anything that is not obviously safe becomes a hyphen. Without +// this a suite named "flagd/rpc" would silently write outside the directory it +// was given. +func reportFileName(name string) string { + var b strings.Builder + for _, r := range name { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_', r == '.': + b.WriteRune(r) + default: + b.WriteRune('-') + } + } + cleaned := strings.Trim(b.String(), "-.") + if cleaned == "" { + cleaned = "report" + } + return cleaned + ".json" +} + +// sdkVersion reports the go-sdk version this binary was built against. +// +// Read from the build info rather than declared, because a declared version is +// a second place to be wrong: the report would keep claiming 1.14.0 after a +// dependency bump moved the actual code underneath it. +func sdkVersion() string { + return moduleVersion(goSDKModule) +} + +func tckVersion() string { + return moduleVersion(tckModule) +} + +func moduleVersion(path string) string { + info, ok := debug.ReadBuildInfo() + if !ok { + return "unknown" + } + if info.Main.Path == path && info.Main.Version != "" { + return info.Main.Version + } + for _, dep := range info.Deps { + if dep == nil || dep.Path != path { + continue + } + // A replace directive means the code being run is not the version the + // requirement names, and saying so is more useful than either version + // alone. + if dep.Replace != nil && dep.Replace.Version != "" { + return dep.Replace.Version + } + if dep.Version != "" { + return dep.Version + } + } + return "unknown" +} + +// featureName turns a Gherkin document's URI into the bare feature name the +// schema asks for: "errors", not "features/errors.feature". +func featureName(uri string) string { + base := filepath.Base(filepath.ToSlash(uri)) + return strings.TrimSuffix(base, filepath.Ext(base)) +} diff --git a/tools/provider-tck/pkg/tck/report_test.go b/tools/provider-tck/pkg/tck/report_test.go new file mode 100644 index 000000000..74e738bec --- /dev/null +++ b/tools/provider-tck/pkg/tck/report_test.go @@ -0,0 +1,195 @@ +package tck_test + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/open-feature/go-sdk-contrib/tools/provider-tck/pkg/tck" + "github.com/open-feature/go-sdk/openfeature" + "github.com/open-feature/go-sdk/openfeature/memprovider" +) + +// TestReportNeverCallsASkippedScenarioPassed is the reason the report exists. +// +// Appendix F requires that a scenario skipped for an undeclared capability is +// reported as skipped with the reason, never as passed. Go's runner does not +// honour that in its own summary: godog counts capability-gated skips in its +// passed tally, so the headline number the suite prints says something false and +// only a separate log line reveals it. +// +// The report is what makes the rule checkable rather than aspirational, so this +// test asserts the property directly: every scenario carrying a tag the suite +// did not declare appears in the report as not-declared, with a reason, and +// none of them appears as passed. +func TestReportNeverCallsASkippedScenarioPassed(t *testing.T) { + dir := t.TempDir() + t.Setenv(tck.ReportDirEnv, dir) + + // Deliberately narrow: declaring only Object leaves every event, lifecycle, + // stale, unavailable and strict-numeric-typing scenario ungated and skipped, + // which is precisely the situation the rule governs. + tck.Run(t, tck.Config{ + Name: "report-selftest", + Control: plainMemoryControl{}, + NewProvider: func(context.Context) (openfeature.FeatureProvider, error) { + return memprovider.NewInMemoryProvider(tck.CanonicalFlagSet()), nil + }, + Capabilities: []tck.Capability{tck.Object}, + }) + + report := readReport(t, filepath.Join(dir, "report-selftest.json")) + + declared := map[string]bool{tck.Object.Tag(): true} + + var skipped, passed int + for _, scenario := range report.Scenarios { + needsUndeclared := false + for _, tag := range scenario.Tags { + // Only tags that gate a capability matter; the feature files are + // free to carry organisational tags that gate nothing. + if _, gates := tck.CapabilityForTag(tag); gates && !declared[tag] { + needsUndeclared = true + } + } + + switch { + case needsUndeclared: + skipped++ + if scenario.Outcome != tck.OutcomeNotDeclared { + t.Errorf("scenario %q needs an undeclared capability but was reported as %q; "+ + "Appendix F requires it be reported as %q and never as passed", + scenario.Name, scenario.Outcome, tck.OutcomeNotDeclared) + } + if scenario.Reason == "" { + t.Errorf("scenario %q was skipped without a reason; the reason is what makes a "+ + "skip readable to someone comparing providers", scenario.Name) + } + case scenario.Outcome == tck.OutcomePassed: + passed++ + default: + t.Errorf("scenario %q needs no undeclared capability but was reported as %q: %s", + scenario.Name, scenario.Outcome, scenario.Reason) + } + } + + if skipped == 0 { + t.Fatal("no scenario was skipped, so this test asserted nothing; either the capability " + + "gate stopped working or the feature files no longer carry capability tags") + } + if passed == 0 { + t.Fatal("no scenario passed, so the suite did not really run") + } + + // The count is the other half of the property. A report that simply omitted + // the scenarios it did not run would satisfy every assertion above while + // still misleading a consumer, who has no way to know how many questions + // went unasked. + if total := len(report.Scenarios); total != skipped+passed { + t.Errorf("report accounts for %d scenarios but %d passed and %d were skipped; "+ + "every scenario in the suite must appear exactly once", total, passed, skipped) + } +} + +// TestReportRecordsUndeclaredCapabilities checks the capability summary agrees +// with the per-scenario detail, since a consumer may read either. +func TestReportRecordsUndeclaredCapabilities(t *testing.T) { + dir := t.TempDir() + t.Setenv(tck.ReportDirEnv, dir) + + tck.Run(t, tck.Config{ + Name: "capability-selftest", + Control: plainMemoryControl{}, + NewProvider: func(context.Context) (openfeature.FeatureProvider, error) { + return memprovider.NewInMemoryProvider(tck.CanonicalFlagSet()), nil + }, + Capabilities: []tck.Capability{tck.Object}, + }) + + report := readReport(t, filepath.Join(dir, "capability-selftest.json")) + + for _, capability := range tck.AllCapabilities() { + result, ok := report.Capabilities[capability.Tag()] + if !ok { + t.Errorf("capability %s is missing from the report; every capability is reported, "+ + "because an absent one is indistinguishable from one that was forgotten", + capability.Tag()) + continue + } + + want := tck.OutcomeNotDeclared + if capability == tck.Object { + want = tck.OutcomePassed + } + if result.State != want { + t.Errorf("capability %s reported as %q, want %q", capability.Tag(), result.State, want) + } + if want == tck.OutcomeNotDeclared && result.Reason == "" { + t.Errorf("capability %s is not declared but carries no reason", capability.Tag()) + } + } +} + +// TestReportNotWrittenByDefault keeps report emission opt-in. +// +// A suite that wrote files into the working directory of every developer who +// ran it would be a nuisance, and worse, a report written by accident is a +// report nobody checked. +func TestReportNotWrittenByDefault(t *testing.T) { + dir := t.TempDir() + t.Setenv(tck.ReportDirEnv, "") + + tck.Run(t, tck.Config{ + Name: "no-report", + Control: plainMemoryControl{}, + NewProvider: func(context.Context) (openfeature.FeatureProvider, error) { + return memprovider.NewInMemoryProvider(tck.CanonicalFlagSet()), nil + }, + Capabilities: []tck.Capability{tck.Object}, + }) + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("reading %s: %v", dir, err) + } + if len(entries) != 0 { + t.Errorf("no report directory was configured but %d file(s) were written", len(entries)) + } +} + +// TestSpecRevisionIsRecorded guards the generated constants. +// +// They are what lets a consumer know which questions a report answers, and they +// are generated rather than written, so the failure mode is silence: a +// regenerate that stopped emitting them would leave the report structurally +// valid and semantically useless. +func TestSpecRevisionIsRecorded(t *testing.T) { + if len(tck.SpecRevision) != 40 { + t.Errorf("SpecRevision = %q, want a 40-character commit SHA; run `make provider-tck-assets`", + tck.SpecRevision) + } + if len(tck.AssetsTree) != 40 { + t.Errorf("AssetsTree = %q, want a 40-character tree SHA; run `make provider-tck-assets`", + tck.AssetsTree) + } +} + +func readReport(t *testing.T, path string) tck.Report { + t.Helper() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("no conformance report at %s: %v", path, err) + } + + var report tck.Report + if err := json.Unmarshal(data, &report); err != nil { + t.Fatalf("report at %s is not valid JSON: %v", path, err) + } + if report.SchemaVersion == "" { + t.Fatalf("report at %s has no schemaVersion", path) + } + return report +} diff --git a/tools/provider-tck/pkg/tck/revision.go b/tools/provider-tck/pkg/tck/revision.go new file mode 100644 index 000000000..db290bee3 --- /dev/null +++ b/tools/provider-tck/pkg/tck/revision.go @@ -0,0 +1,15 @@ +// Code generated by sync_assets.go. DO NOT EDIT. + +package tck + +// SpecRevision is the open-feature/spec commit the embedded conformance +// artifacts were taken from. +const SpecRevision = "dfa16586d91ca020ef1b3b82a7c972d833ff8f29" + +// AssetsTree is the git tree object ID of specification/assets/provider-tck at +// SpecRevision. +// +// It identifies the artifacts rather than the commit, so an unrelated change +// elsewhere in the specification leaves it untouched, and +// `git rev-parse SpecRevision:specification/assets/provider-tck` reproduces it. +const AssetsTree = "904aa7d5fd7a856a4f92ace24355bd1987143abc" diff --git a/tools/provider-tck/pkg/tck/run.go b/tools/provider-tck/pkg/tck/run.go index fc6ac9aed..c6e8f591a 100644 --- a/tools/provider-tck/pkg/tck/run.go +++ b/tools/provider-tck/pkg/tck/run.go @@ -8,6 +8,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/cucumber/godog" "github.com/open-feature/go-sdk/openfeature" @@ -67,6 +68,7 @@ func Run(t *testing.T, cfg Config) { }.Run() r.reportSkips() + r.writeReport() if status != 0 && !t.Failed() { t.Fatalf("provider-tck [%s]: suite failed with exit status %d", cfg.Name, status) @@ -81,6 +83,29 @@ type runner struct { mu sync.Mutex skips []skippedScenario + + // records is the per-scenario outcome list the conformance report is built + // from. It is kept even when no report is requested, because it costs + // nothing and the alternative is a code path that only ever runs in CI. + records []scenarioRecord + + // started times scenarios by name. godog gives no scenario-scoped place to + // hang this, and scenarios run serially, so a map keyed by name is enough. + started map[string]time.Time + + // gated names the scenarios the capability gate stopped before their first + // step, so the after hook does not record them a second time. + // + // It exists because godog does not deliver the before hook's ErrSkip to the + // after hook -- err arrives nil, indistinguishable from a scenario that ran + // and passed. Testing for ErrSkip there silently recorded every skipped + // scenario twice, once correctly and once as passed, which is the exact + // failure Appendix F forbids. + gated map[string]bool + + // providerName is what the provider called itself, observed from the last + // scenario that registered one. + providerName string } // skippedScenario records a scenario that did not run because the provider did @@ -108,11 +133,16 @@ func (r *runner) initializeScenario(ctx *godog.ScenarioContext) { func (r *runner) beforeScenario(ctx context.Context, sc *godog.Scenario) (context.Context, error) { if capability, missing := r.missingCapability(sc); missing { r.recordSkip(sc.Name, capability) + r.recordOutcome(sc, OutcomeNotDeclared, fmt.Sprintf( + "requires capability %s, which this provider does not declare", capability.Tag()), 0) + r.markGated(sc.Name) return ctx, fmt.Errorf( "%w: scenario requires capability %s (Gherkin tag %s), which this provider does not declare. Declared capabilities: %s", godog.ErrSkip, capability, capability.Tag(), formatCapabilities(r.caps.sorted())) } + r.markStarted(sc.Name) + ctx = withState(ctx, newScenarioState(&r.cfg)) if err := r.cfg.Control.PrepareScenario(ctx); err != nil { @@ -125,13 +155,89 @@ func (r *runner) beforeScenario(ctx context.Context, sc *godog.Scenario) (contex // afterScenario detaches the scenario's event handlers. A skipped scenario has // no state, which is not an error. -func (r *runner) afterScenario(ctx context.Context, _ *godog.Scenario, err error) (context.Context, error) { +func (r *runner) afterScenario(ctx context.Context, sc *godog.Scenario, err error) (context.Context, error) { if state, stateErr := stateFrom(ctx); stateErr == nil { + if state.providerName != "" { + r.mu.Lock() + r.providerName = state.providerName + r.mu.Unlock() + } state.teardown() } + + // A capability skip was already recorded before the scenario started. + // Recording it again here would put it in the report twice, the second time + // as passed. + if !r.wasGated(sc.Name) { + outcome, reason := OutcomePassed, "" + if err != nil { + outcome, reason = OutcomeFailed, err.Error() + } + r.recordOutcome(sc, outcome, reason, r.elapsed(sc.Name)) + } + return ctx, err } +// recordOutcome appends one scenario's result. +func (r *runner) recordOutcome(sc *godog.Scenario, outcome Outcome, reason string, duration time.Duration) { + tags := make([]string, 0, len(sc.Tags)) + for _, tag := range sc.Tags { + tags = append(tags, tag.Name) + } + + r.mu.Lock() + defer r.mu.Unlock() + r.records = append(r.records, scenarioRecord{ + feature: featureName(sc.Uri), + name: sc.Name, + tags: tags, + outcome: outcome, + reason: reason, + duration: duration, + }) +} + +// markGated and markStarted create their maps on first use. +// +// The runner has to work as a zero value: it is constructed as a struct literal +// in tests that exercise the gate directly, and a nil map assignment there is a +// panic rather than a helpful failure. +func (r *runner) markGated(name string) { + r.mu.Lock() + defer r.mu.Unlock() + if r.gated == nil { + r.gated = map[string]bool{} + } + r.gated[name] = true +} + +func (r *runner) markStarted(name string) { + r.mu.Lock() + defer r.mu.Unlock() + if r.started == nil { + r.started = map[string]time.Time{} + } + r.started[name] = time.Now() +} + +// wasGated reports whether the capability gate stopped this scenario. +func (r *runner) wasGated(name string) bool { + r.mu.Lock() + defer r.mu.Unlock() + return r.gated[name] +} + +func (r *runner) elapsed(name string) time.Duration { + r.mu.Lock() + defer r.mu.Unlock() + start, ok := r.started[name] + if !ok { + return 0 + } + return time.Since(start) +} + // missingCapability reports the first capability a scenario needs that the // provider did not declare. // @@ -139,7 +245,7 @@ func (r *runner) afterScenario(ctx context.Context, _ *godog.Scenario, err error // files stay free to carry organisational tags. func (r *runner) missingCapability(sc *godog.Scenario) (Capability, bool) { for _, tag := range sc.Tags { - capability, gates := capabilityForTag(tag.Name) + capability, gates := CapabilityForTag(tag.Name) if !gates { continue } diff --git a/tools/provider-tck/pkg/tck/state.go b/tools/provider-tck/pkg/tck/state.go index cb3c1cebb..3671d0b9d 100644 --- a/tools/provider-tck/pkg/tck/state.go +++ b/tools/provider-tck/pkg/tck/state.go @@ -47,6 +47,12 @@ type evaluation struct { type scenarioState struct { cfg *Config + // providerName is the name the provider reports through its own metadata, + // captured so the conformance report identifies the provider rather than the + // suite. Config.Name is chosen to read well in failure messages -- "flagd-rpc" + // -- and is the configuration, not the provider. + providerName string + client *openfeature.Client flag *flagUnderTest diff --git a/tools/provider-tck/pkg/tck/steps_provider.go b/tools/provider-tck/pkg/tck/steps_provider.go index 0c9bc0c6b..c834864bd 100644 --- a/tools/provider-tck/pkg/tck/steps_provider.go +++ b/tools/provider-tck/pkg/tck/steps_provider.go @@ -49,6 +49,7 @@ func aStableProvider(ctx context.Context) (context.Context, error) { state.cfg.readyTimeout(), regErr) } + state.providerName = provider.Metadata().Name state.client = openfeature.NewClient(state.cfg.domain()) return ctx, nil } diff --git a/tools/provider-tck/sync_assets.go b/tools/provider-tck/sync_assets.go index 03a88b3d0..1889ff690 100644 --- a/tools/provider-tck/sync_assets.go +++ b/tools/provider-tck/sync_assets.go @@ -39,23 +39,35 @@ import ( "fmt" "io/fs" "os" + "os/exec" "path/filepath" + "strings" ) const ( - specRoot = "pkg/tck/spec/specification/assets/provider-tck" + // specRoot is the checked-out submodule. + specRoot = "pkg/tck/spec" + + // assetsPathInSpec is where the artifacts live inside the specification + // repository. It names both the directory to copy from and the tree whose + // hash is recorded alongside the commit. + assetsPathInSpec = "specification/assets/provider-tck" + destRoot = "pkg/tck/assets" ) +// specAssets is the directory the artifacts are copied from. +var specAssets = filepath.Join(specRoot, filepath.FromSlash(assetsPathInSpec)) + // copies maps a directory in the spec repository to its destination in the // package. The layout is preserved rather than renamed, so a reader comparing the // two trees sees the same shape. var copies = []string{"gherkin", "flags", "openapi"} func main() { - if _, err := os.Stat(specRoot); err != nil { + if _, err := os.Stat(specAssets); err != nil { fail("the spec submodule is not checked out at %s: %v\n\n"+ - "Run: git submodule update --init tools/provider-tck/pkg/tck/spec", specRoot, err) + "Run: git submodule update --init tools/provider-tck/pkg/tck/spec", specAssets, err) } if err := os.RemoveAll(destRoot); err != nil { @@ -68,14 +80,18 @@ func main() { total := 0 for _, dir := range copies { - n, err := copyTree(filepath.Join(specRoot, dir), filepath.Join(destRoot, dir)) + n, err := copyTree(filepath.Join(specAssets, dir), filepath.Join(destRoot, dir)) if err != nil { fail("copying %s: %v", dir, err) } total += n } - fmt.Printf("synced %d conformance artifacts from %s\n", total, specRoot) + if err := writeRevision(); err != nil { + fail("recording the spec revision: %v", err) + } + + fmt.Printf("synced %d conformance artifacts from %s\n", total, specAssets) } // gitattributes pins the line endings of everything generated here. @@ -146,3 +162,57 @@ func fail(format string, args ...any) { fmt.Fprintf(os.Stderr, "sync_assets: "+format+"\n", args...) os.Exit(1) } + +// writeRevision records which revision of the specification these artifacts came +// from, as Go source. +// +// A conformance report has to name the revision it ran against or two reports +// cannot be compared, and for Go the submodule is absent from the published +// module, so the answer is baked in at sync time rather than read at run time. +// Generating it from the same command that copies the artifacts is what keeps it +// honest: the CI check regenerates both and fails on any difference, so a +// revision that disagrees with the artifacts beside it cannot be committed. +// +// The tree hash is recorded as well as the commit because it identifies the +// artifacts alone. It does not change when an unrelated part of the +// specification does, so two runs that executed identical artifacts report the +// same value even when pinned to different commits. It is also checkable, since +// `git rev-parse :specification/assets/provider-tck` must reproduce it. +func writeRevision() error { + commit, err := gitOutput("-C", specRoot, "rev-parse", "HEAD") + if err != nil { + return err + } + tree, err := gitOutput("-C", specRoot, "rev-parse", "HEAD:"+assetsPathInSpec) + if err != nil { + return err + } + + return os.WriteFile(filepath.Join("pkg", "tck", "revision.go"), + []byte(fmt.Sprintf(revisionTemplate, commit, tree)), 0o644) +} + +func gitOutput(args ...string) (string, error) { + out, err := exec.Command("git", args...).Output() + if err != nil { + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return strings.TrimSpace(string(out)), nil +} + +const revisionTemplate = `// Code generated by sync_assets.go. DO NOT EDIT. + +package tck + +// SpecRevision is the open-feature/spec commit the embedded conformance +// artifacts were taken from. +const SpecRevision = %q + +// AssetsTree is the git tree object ID of specification/assets/provider-tck at +// SpecRevision. +// +// It identifies the artifacts rather than the commit, so an unrelated change +// elsewhere in the specification leaves it untouched, and +// ` + "`git rev-parse SpecRevision:specification/assets/provider-tck`" + ` reproduces it. +const AssetsTree = %q +` From 99033ab33d65994aa1f581dad3cd693c57efdcdf Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 24 Aug 2026 17:58:40 +0200 Subject: [PATCH 2/5] fix(provider-tck): give a failed capability a reason, and stop claiming untested ones Two defects in the capability rollup, both found by the Java implementation reviewing this one. A failed capability was emitted as {"state": "failed"} with no reason. The schema now requires a reason for any outcome other than passed, so that entry does not validate -- and it would have appeared only when a provider was actually failing, which is precisely when the report matters. It now says how many of how many scenarios carrying the tag failed, and points at the per-scenario results for which and why. No test caught it because every self-test suite passes, so nothing that runs end to end ever reaches that branch. The new internal test drives the report builder directly with synthetic records, which is the only way to exercise a failure without breaking a provider on purpose. A declared capability that no scenario carries was reported as passed. @targeting is reserved -- it exists in the vocabulary but nothing tests it, because asserting that an evaluation context reached the backend needs an echo operation the control API does not have -- so a provider declaring it got a green result for a claim nothing had examined. That is the vacuous pass the capability vocabulary was introduced to eliminate, arriving through the report rather than through the suite. Such a capability is now omitted. The suite asked no question, so it has no answer to report, and a consumer sees the tag is absent rather than a pass it cannot rely on. Omitting is preferred to inventing a fifth outcome: the four in the schema are about what the provider did, and "the suite does not test this" is a fact about the suite. Signed-off-by: Simon Schrottner --- tools/provider-tck/pkg/tck/report.go | 39 ++++++++--- .../pkg/tck/report_internal_test.go | 68 +++++++++++++++++++ tools/provider-tck/pkg/tck/report_test.go | 49 ++++++++++++- 3 files changed, 143 insertions(+), 13 deletions(-) create mode 100644 tools/provider-tck/pkg/tck/report_internal_test.go diff --git a/tools/provider-tck/pkg/tck/report.go b/tools/provider-tck/pkg/tck/report.go index 17e4bcfc0..60ccf46fe 100644 --- a/tools/provider-tck/pkg/tck/report.go +++ b/tools/provider-tck/pkg/tck/report.go @@ -131,10 +131,15 @@ func (r *runner) buildReport() Report { }) scenarios := make([]ReportScenario, 0, len(records)) - // failedCapabilities tracks which declared capabilities had a scenario fail, - // so a capability is only reported as passed when everything gating on it - // actually passed. - failedCapabilities := map[Capability]bool{} + // failed counts, per capability, the scenarios gating on it that failed, so a + // capability is reported as passed only when everything gating on it passed and + // a failure can say how much failed. + failed := map[Capability]int{} + // exercised counts the scenarios gating on each capability at all. A capability + // no scenario carries cannot have been demonstrated, and reporting it as passed + // would claim conformance the suite never tested -- which is the same vacuous + // green the capability vocabulary exists to prevent. + exercised := map[Capability]int{} for _, rec := range records { scenarios = append(scenarios, ReportScenario{ @@ -145,12 +150,14 @@ func (r *runner) buildReport() Report { Reason: rec.reason, DurationMs: float64(rec.duration.Microseconds()) / 1000.0, }) - if rec.outcome != OutcomeFailed { - continue - } for _, tag := range rec.tags { - if capability, gates := CapabilityForTag(tag); gates { - failedCapabilities[capability] = true + capability, gates := CapabilityForTag(tag) + if !gates { + continue + } + exercised[capability]++ + if rec.outcome == OutcomeFailed { + failed[capability]++ } } } @@ -165,8 +172,18 @@ func (r *runner) buildReport() Report { "not declared by this provider's configuration; the %s scenarios were skipped and did not contribute to this result", capability.Tag()), } - case failedCapabilities[capability]: - capabilities[capability.Tag()] = ReportCapability{State: OutcomeFailed} + case exercised[capability] == 0: + // Declared, but no scenario in the suite gates on it. Saying nothing is + // the only honest answer: the suite asked no question, so it has none + // to report. Claiming passed would be a green result for an untested + // claim, which is precisely what this suite exists to make impossible. + case failed[capability] > 0: + capabilities[capability.Tag()] = ReportCapability{ + State: OutcomeFailed, + Reason: fmt.Sprintf( + "%d of %d scenarios carrying %s failed; the per-scenario results say which, and why", + failed[capability], exercised[capability], capability.Tag()), + } default: capabilities[capability.Tag()] = ReportCapability{State: OutcomePassed} } diff --git a/tools/provider-tck/pkg/tck/report_internal_test.go b/tools/provider-tck/pkg/tck/report_internal_test.go new file mode 100644 index 000000000..094b026f0 --- /dev/null +++ b/tools/provider-tck/pkg/tck/report_internal_test.go @@ -0,0 +1,68 @@ +package tck + +import ( + "strings" + "testing" +) + +// TestFailedCapabilityCarriesAReason exercises the branch no passing suite can. +// +// The schema requires a reason whenever an outcome is not "passed", and the +// self-test suites all pass, so nothing that runs end to end ever builds a failed +// capability entry. That branch was schema-invalid for a while and no test +// noticed, because the only way to reach it is to fail a scenario on purpose -- +// which is what this does, by handing the report builder the records directly +// rather than by breaking a provider. +func TestFailedCapabilityCarriesAReason(t *testing.T) { + caps, err := newCapabilitySet([]Capability{Object, Events}) + if err != nil { + t.Fatalf("building the capability set: %v", err) + } + + r := &runner{cfg: Config{Name: "synthetic", Control: stubControl{}}, caps: caps} + r.records = []scenarioRecord{ + {feature: "errors", name: "a structured flag fails", tags: []string{"@object"}, + outcome: OutcomeFailed, reason: "resolved to nil, expected an object"}, + {feature: "errors", name: "a structured flag succeeds", tags: []string{"@object"}, + outcome: OutcomePassed}, + {feature: "events", name: "ready fires", tags: []string{"@events"}, + outcome: OutcomePassed}, + } + + report := r.buildReport() + + object, present := report.Capabilities[Object.Tag()] + if !present { + t.Fatalf("%s is missing from the report", Object.Tag()) + } + if object.State != OutcomeFailed { + t.Errorf("%s reported as %q, want %q", Object.Tag(), object.State, OutcomeFailed) + } + if object.Reason == "" { + t.Fatalf("%s failed but carries no reason; the schema rejects a non-passed outcome "+ + "without one, so a report built this way would not validate", Object.Tag()) + } + // The reason has to be usable, not merely present: a consumer reading a + // comparison page wants to know how much failed before opening the detail. + if !strings.Contains(object.Reason, "1 of 2") { + t.Errorf("%s reason %q does not say how many of how many failed", Object.Tag(), object.Reason) + } + + events, present := report.Capabilities[Events.Tag()] + if !present { + t.Fatalf("%s is missing from the report", Events.Tag()) + } + if events.State != OutcomePassed { + t.Errorf("%s reported as %q, want %q; one capability failing must not drag down another", + Events.Tag(), events.State, OutcomePassed) + } + + // Every capability the report does mention, other than a pass, must carry a + // reason -- the same rule the schema enforces, checked here so a change to the + // builder fails in this package rather than in a validator downstream. + for tag, result := range report.Capabilities { + if result.State != OutcomePassed && result.Reason == "" { + t.Errorf("capability %s is %q with no reason", tag, result.State) + } + } +} diff --git a/tools/provider-tck/pkg/tck/report_test.go b/tools/provider-tck/pkg/tck/report_test.go index 74e738bec..c1ff793d4 100644 --- a/tools/provider-tck/pkg/tck/report_test.go +++ b/tools/provider-tck/pkg/tck/report_test.go @@ -113,8 +113,8 @@ func TestReportRecordsUndeclaredCapabilities(t *testing.T) { for _, capability := range tck.AllCapabilities() { result, ok := report.Capabilities[capability.Tag()] if !ok { - t.Errorf("capability %s is missing from the report; every capability is reported, "+ - "because an absent one is indistinguishable from one that was forgotten", + t.Errorf("capability %s is missing from the report; a capability is omitted only when "+ + "it is declared and no scenario exercises it, which is not the case here", capability.Tag()) continue } @@ -193,3 +193,48 @@ func readReport(t *testing.T, path string) tck.Report { } return report } + +// TestReservedCapabilityIsNotReportedAsPassed covers the capability a provider +// declares and the suite never tests. +// +// @targeting is reserved: it exists in the vocabulary but no scenario carries +// it, because asserting that an evaluation context reached the backend needs an +// echo operation the control API does not have yet. Reporting it as passed would +// be a green result for a claim nothing tested -- the same vacuous pass the +// capability vocabulary was introduced to eliminate, arriving through the report +// instead of through the suite. +// +// Omitting it is the honest answer: the suite asked no question, so it has none +// to report. A consumer sees the tag is absent rather than a pass it cannot rely +// on. +func TestReservedCapabilityIsNotReportedAsPassed(t *testing.T) { + dir := t.TempDir() + t.Setenv(tck.ReportDirEnv, dir) + + tck.Run(t, tck.Config{ + Name: "reserved-capability", + Control: plainMemoryControl{}, + NewProvider: func(context.Context) (openfeature.FeatureProvider, error) { + return memprovider.NewInMemoryProvider(tck.CanonicalFlagSet()), nil + }, + // Targeting is declared and no scenario carries it. Object is declared so + // the suite still does something. + Capabilities: []tck.Capability{tck.Object, tck.Targeting}, + }) + + report := readReport(t, filepath.Join(dir, "reserved-capability.json")) + + if result, present := report.Capabilities[tck.Targeting.Tag()]; present { + t.Errorf("%s was declared and no scenario exercises it, but the report states %q; "+ + "a capability the suite never tested must not be reported as a result", + tck.Targeting.Tag(), result.State) + } + + // The declared capability that is exercised must still be reported, so the + // omission above is specific rather than a general failure to report. + if result, present := report.Capabilities[tck.Object.Tag()]; !present { + t.Errorf("%s was declared and exercised but is missing from the report", tck.Object.Tag()) + } else if result.State != tck.OutcomePassed { + t.Errorf("%s reported as %q, want %q", tck.Object.Tag(), result.State, tck.OutcomePassed) + } +} From bb3a394c91a1a3f04fe009d6489065308af1110c Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 24 Aug 2026 21:24:37 +0200 Subject: [PATCH 3/5] feat(provider-tck): identify a Scenario Outline row by its parameters A scenario entry in the conformance report was identified by feature and name. Every row of a Scenario Outline shares one name, so the type-mismatch matrix in errors.feature produced eleven entries that differed only in durationMs. If one row failed and ten passed, the report could not say which failed, and a consumer keying on feature and name kept whichever row it read last. Per the report schema, a scenarioResult now carries `example`: the Examples row it came from, keyed by column header, with the cells verbatim as strings. Gherkin has no types, so "1" stays "1" rather than becoming 1 -- the report says what the table said. It is present only for outline rows and omitted otherwise. godog hands a hook an already-expanded pickle, whose step text has the parameters substituted in and whose row is otherwise gone. What survives is AstNodeIds, whose last entry is the id of the Examples TableRow. The row is therefore recovered by parsing the embedded feature files a second time and indexing every TableRow by that id. Those ids come from a counter godog creates once per run and shares across the files it parses, so reproducing them means reproducing godog's parse -- same files, same order, pickle compilation in between. That coupling is not left to be trusted: a pickle that came from an outline and did not resolve fails the run, because quietly returning to the ambiguity this field exists to remove is worse than a build failure. The capability gate records its outcome before a scenario starts, so it fills the field in too. Four skipped rows of the @object outline are as ambiguous as four failed ones. The gate's own bookkeeping is keyed by pickle id rather than by scenario name for the same reason. Gherkin allows an Examples block to carry its own tags, so two rows of one outline can differ in whether the gate stops them; keyed by name, gating one row suppressed the after hook for every row and the rows that did run would have vanished from the report. gherkin/go/v26 moves from an indirect requirement to a direct one. It is the same module and version godog already builds against, so no dependency is added and no go.sum entry changes. Verified against the schema on open-feature/spec#425 with a Draft 2020-12 validator: every report the self-tests emit validates, the eleven matrix rows carry eleven distinct examples matching the feature file, and (feature, name, example) is unique across every scenario in a report. Signed-off-by: Simon Schrottner --- tools/provider-tck/README.md | 38 ++++ tools/provider-tck/go.mod | 2 +- tools/provider-tck/pkg/tck/examples.go | 231 ++++++++++++++++++++++ tools/provider-tck/pkg/tck/report.go | 48 +++-- tools/provider-tck/pkg/tck/report_test.go | 153 ++++++++++++++ tools/provider-tck/pkg/tck/run.go | 109 ++++++++-- 6 files changed, 555 insertions(+), 26 deletions(-) create mode 100644 tools/provider-tck/pkg/tck/examples.go diff --git a/tools/provider-tck/README.md b/tools/provider-tck/README.md index 8018d96ab..b90b693db 100644 --- a/tools/provider-tck/README.md +++ b/tools/provider-tck/README.md @@ -260,6 +260,44 @@ artifacts report the same value even when pinned to different commits — and it *configuration*, and it is reported as such. One provider with two materially different modes produces two reports that are not interchangeable. +### What identifies a scenario + +`feature` and `name` together do not. Every row of a Scenario Outline shares one name, and the +type-mismatch matrix in `errors.feature` is eleven rows, so eleven entries carry the same feature and +the same name. A report that stopped there could not say which row failed, and a consumer keying on +the pair would keep whichever row it read last. + +A row is identified by its parameters, which the report carries in `example` — the Examples row it +came from, keyed by column header: + +```console +$ jq -c '.scenarios[] | select(.name | startswith("Requesting the wrong type")) | .example' reports/in-memory.json +{"default":"false","key":"string-flag","requested":"Boolean"} +{"default":"1","key":"string-flag","requested":"Integer"} +{"default":"0.1","key":"string-flag","requested":"Float"} +... +``` + +The values are the cells verbatim, as strings. Gherkin has no types, so `"1"` stays `"1"`: coercing +it would be this implementation inventing a fact the feature file did not state, and four +implementations would each invent a different one. + +It is a field rather than a naming convention because the parameters *are* the identity, and they +come from the feature file rather than from any runner. Mandating a mangled name instead would put a +separator, an ordering and an escaping rule into normative text that every implementation has to +reproduce byte for byte, and drift there is invisible until two reports silently fail to line up. + +A skipped row carries it too. The capability gate records its outcome before the scenario starts, so +the four rows of the `@object` outline would otherwise be four `not-declared` entries differing in +nothing — exactly as ambiguous as four failures. + +godog's hooks receive an already-expanded scenario, whose step text has the parameters substituted +into it and whose row is otherwise gone. What survives is `AstNodeIds`, whose last entry is the id of +the Examples `TableRow`, so the row is recovered by parsing the embedded feature files a second time +and indexing every row by that id. Those ids come from a counter godog shares across the files it +parses, which means reproducing them means reproducing godog's parse; a run that cannot resolve a row +it knows came from an outline fails rather than quietly emitting the ambiguity again. + ## The self-tests Three suites run against providers from the SDK itself. They need no Docker and finish in diff --git a/tools/provider-tck/go.mod b/tools/provider-tck/go.mod index ea5692471..29b18a5fd 100644 --- a/tools/provider-tck/go.mod +++ b/tools/provider-tck/go.mod @@ -3,13 +3,13 @@ module github.com/open-feature/go-sdk-contrib/tools/provider-tck go 1.25.0 require ( + github.com/cucumber/gherkin/go/v26 v26.2.0 github.com/cucumber/godog v0.15.1 github.com/cucumber/messages/go/v21 v21.0.1 github.com/open-feature/go-sdk v1.18.0 ) require ( - github.com/cucumber/gherkin/go/v26 v26.2.0 // indirect github.com/gofrs/uuid v4.4.0+incompatible // indirect github.com/hashicorp/go-immutable-radix v1.3.1 // indirect github.com/hashicorp/go-memdb v1.3.5 // indirect diff --git a/tools/provider-tck/pkg/tck/examples.go b/tools/provider-tck/pkg/tck/examples.go new file mode 100644 index 000000000..4a53f2b2d --- /dev/null +++ b/tools/provider-tck/pkg/tck/examples.go @@ -0,0 +1,231 @@ +package tck + +import ( + "fmt" + "io/fs" + "sort" + "strings" + "sync" + + "github.com/cucumber/gherkin/go/v26" + "github.com/cucumber/godog" + messages "github.com/cucumber/messages/go/v21" +) + +// WHY THIS FILE EXISTS +// +// A row of a Scenario Outline is identified by its parameters. Every row shares +// the outline's name, so a report that identifies a scenario by feature and name +// gives eleven identical entries for the eleven rows of the type-mismatch matrix +// in errors.feature. If one row fails and ten pass, that report cannot say which +// failed, and a consumer keying on feature and name keeps whichever row it read +// last. +// +// godog hands a hook a *godog.Scenario, which is a messages.Pickle. A pickle is +// the already-expanded scenario: its step text has the parameters substituted +// into it, and the row they came from is gone. What survives is AstNodeIds, +// whose last entry, for a pickle compiled from an outline, is the id of the +// Examples TableRow the pickle was expanded from. +// +// So the row is recovered by parsing the same feature files a second time and +// indexing every Examples TableRow by that id. + +// exampleRow is one row of an Examples table. +type exampleRow struct { + // values are the cells keyed by column header, verbatim as strings. Gherkin + // has no types, so "1" stays "1": coercing it to a number would be this + // implementation inventing a fact the feature file did not state, and the + // four language implementations would each invent a different one. + values map[string]string + // order is where the row sits among all the rows of its scenario, counting + // across every Examples block the scenario has. It is carried so the report + // can list the rows in the order the table declares them rather than in + // whatever order sorting the parameter values happens to produce. + order int +} + +// exampleIndex resolves a pickle to the Examples row it was expanded from. +type exampleIndex struct { + // rows is keyed by the id of the TableRow AST node. + rows map[string]exampleRow + // outlines names every scenario that is an outline, keyed by feature URI and + // scenario name. It exists so a failure to resolve a row can be told apart + // from a scenario that legitimately has none, and reported rather than + // silently emitting a report with the ambiguity this field exists to remove. + outlines map[string]bool +} + +var ( + exampleIndexOnce sync.Once + exampleIndexVal *exampleIndex + exampleIndexErr error +) + +// scenarioExamples returns the index, building it once per process. +func scenarioExamples() (*exampleIndex, error) { + exampleIndexOnce.Do(func() { + exampleIndexVal, exampleIndexErr = buildExampleIndex() + }) + return exampleIndexVal, exampleIndexErr +} + +// buildExampleIndex parses the embedded feature files and indexes their +// Examples rows by AST node id. +// +// The id has to agree with the one godog will report, and godog's ids are not +// intrinsic to a document: they come from a counter (messages.Incrementing) that +// godog creates once per run and shares across every file it parses, so an id +// depends on how many nodes were numbered before it. Reproducing them therefore +// means reproducing godog's whole parse — the same files, in the same order, +// with pickle compilation in between, because compiling pickles draws from the +// same counter. +// +// That is a coupling to godog's internals, and it is a deliberate one: the +// alternative is to fork the pickle compiler. It is not left to be trusted. +// A pickle that comes from an outline and does not resolve is reported as a +// failure by the run, so a godog release that renumbers nodes breaks the build +// loudly instead of quietly emitting reports with the ambiguity removed again. +func buildExampleIndex() (*exampleIndex, error) { + index := &exampleIndex{ + rows: map[string]exampleRow{}, + outlines: map[string]bool{}, + } + + paths, err := featureFilePaths() + if err != nil { + return nil, err + } + + newID := (&messages.Incrementing{}).NewId + for _, path := range paths { + file, err := assets.Open(path) + if err != nil { + return nil, fmt.Errorf("opening the embedded feature file %s: %w", path, err) + } + document, err := gherkin.ParseGherkinDocumentForLanguage(file, gherkin.DefaultDialect, newID) + closeErr := file.Close() + if err != nil { + return nil, fmt.Errorf("parsing the embedded feature file %s: %w", path, err) + } + if closeErr != nil { + return nil, fmt.Errorf("closing the embedded feature file %s: %w", path, closeErr) + } + + document.Uri = path + index.collectDocument(document) + + // The result is discarded; the side effect is the point. Compiling the + // pickles advances the id counter exactly as godog's parse advances it, + // so the next document's AST nodes are numbered the way godog numbers + // them. + _ = gherkin.Pickles(*document, path, newID) + } + + return index, nil +} + +// featureFilePaths lists the embedded feature files in the order godog walks +// them, which is the lexical order fs.WalkDir yields. +func featureFilePaths() ([]string, error) { + var paths []string + err := fs.WalkDir(assets, featuresPath, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + if entry.IsDir() || !strings.HasSuffix(path, ".feature") { + return nil + } + paths = append(paths, path) + return nil + }) + if err != nil { + return nil, fmt.Errorf("listing the embedded feature files: %w", err) + } + // WalkDir already yields lexical order; sorting says so rather than relying + // on it, since the ids only line up if this order matches godog's. + sort.Strings(paths) + return paths, nil +} + +func (index *exampleIndex) collectDocument(document *messages.GherkinDocument) { + if document == nil || document.Feature == nil { + return + } + for _, child := range document.Feature.Children { + if child == nil { + continue + } + if child.Scenario != nil { + index.collectScenario(document.Uri, child.Scenario) + } + if child.Rule == nil { + continue + } + for _, ruleChild := range child.Rule.Children { + if ruleChild != nil && ruleChild.Scenario != nil { + index.collectScenario(document.Uri, ruleChild.Scenario) + } + } + } +} + +func (index *exampleIndex) collectScenario(uri string, scenario *messages.Scenario) { + if len(scenario.Examples) == 0 { + return + } + index.outlines[outlineKey(uri, scenario.Name)] = true + + order := 0 + for _, examples := range scenario.Examples { + if examples == nil || examples.TableHeader == nil { + continue + } + headers := make([]string, 0, len(examples.TableHeader.Cells)) + for _, cell := range examples.TableHeader.Cells { + headers = append(headers, cell.Value) + } + + for _, row := range examples.TableBody { + if row == nil { + continue + } + values := make(map[string]string, len(headers)) + for i, cell := range row.Cells { + // A row with more cells than headers is malformed Gherkin the + // parser would have rejected; guarding costs nothing and keeps a + // future parser change from panicking here. + if i >= len(headers) { + break + } + values[headers[i]] = cell.Value + } + if len(values) > 0 { + index.rows[row.Id] = exampleRow{values: values, order: order} + } + order++ + } + } +} + +// rowFor resolves the Examples row a pickle was expanded from. +// +// The last AST node id is the TableRow for an outline pickle and the Scenario +// node for an ordinary one, so a lookup that misses is the ordinary case rather +// than an error. +func (index *exampleIndex) rowFor(sc *godog.Scenario) (exampleRow, bool) { + if sc == nil || len(sc.AstNodeIds) == 0 { + return exampleRow{}, false + } + row, ok := index.rows[sc.AstNodeIds[len(sc.AstNodeIds)-1]] + return row, ok +} + +// isOutline reports whether a scenario name in a feature belongs to a Scenario +// Outline, and therefore must carry an example. +func (index *exampleIndex) isOutline(uri, name string) bool { + return index.outlines[outlineKey(uri, name)] +} + +func outlineKey(uri, name string) string { + return uri + "\n" + name +} diff --git a/tools/provider-tck/pkg/tck/report.go b/tools/provider-tck/pkg/tck/report.go index 60ccf46fe..52f5bbe0e 100644 --- a/tools/provider-tck/pkg/tck/report.go +++ b/tools/provider-tck/pkg/tck/report.go @@ -90,22 +90,40 @@ type ReportCapability struct { } type ReportScenario struct { - Feature string `json:"feature"` - Name string `json:"name"` - Tags []string `json:"tags,omitempty"` - Outcome Outcome `json:"outcome"` - Reason string `json:"reason,omitempty"` - DurationMs float64 `json:"durationMs,omitempty"` + Feature string `json:"feature"` + Name string `json:"name"` + // Example is the Examples row this entry came from, keyed by column header, + // present only for a scenario expanded from a Scenario Outline. + // + // It is what makes such an entry identifiable. Feature and name are shared by + // every row of an outline -- eleven rows of the type-mismatch matrix in + // errors.feature produce eleven otherwise identical entries -- so without it + // a report cannot say which row failed. + // + // The values are the cells verbatim, as strings. Gherkin has no types, so the + // cell "1" is reported as "1" and not as 1; the report says what the table + // said and leaves the interpretation to whoever reads it. + Example map[string]string `json:"example,omitempty"` + Tags []string `json:"tags,omitempty"` + Outcome Outcome `json:"outcome"` + Reason string `json:"reason,omitempty"` + DurationMs float64 `json:"durationMs,omitempty"` } // scenarioRecord is what the runner accumulates as scenarios execute. type scenarioRecord struct { - feature string - name string - tags []string - outcome Outcome - reason string - duration time.Duration + feature string + name string + example map[string]string + // exampleOrder is the row's position in its scenario's Examples tables, kept + // only to sort the report. Ordering by the parameter values would list the + // rows of a matrix in an order the feature file never mentions, which makes a + // report needlessly hard to read next to the table it came from. + exampleOrder int + tags []string + outcome Outcome + reason string + duration time.Duration } // buildReport assembles the report from what the run observed. @@ -127,7 +145,10 @@ func (r *runner) buildReport() Report { if records[i].feature != records[j].feature { return records[i].feature < records[j].feature } - return records[i].name < records[j].name + if records[i].name != records[j].name { + return records[i].name < records[j].name + } + return records[i].exampleOrder < records[j].exampleOrder }) scenarios := make([]ReportScenario, 0, len(records)) @@ -145,6 +166,7 @@ func (r *runner) buildReport() Report { scenarios = append(scenarios, ReportScenario{ Feature: rec.feature, Name: rec.name, + Example: rec.example, Tags: rec.tags, Outcome: rec.outcome, Reason: rec.reason, diff --git a/tools/provider-tck/pkg/tck/report_test.go b/tools/provider-tck/pkg/tck/report_test.go index c1ff793d4..765284b6f 100644 --- a/tools/provider-tck/pkg/tck/report_test.go +++ b/tools/provider-tck/pkg/tck/report_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "os" "path/filepath" + "sort" + "strings" "testing" "github.com/open-feature/go-sdk-contrib/tools/provider-tck/pkg/tck" @@ -176,6 +178,157 @@ func TestSpecRevisionIsRecorded(t *testing.T) { } } +// TestOutlineRowsAreDistinguishable is the property the example field exists to +// establish. +// +// Every row of a Scenario Outline shares one feature and one name. The +// type-mismatch matrix in errors.feature is eleven rows, so before the field +// existed the report held eleven entries differing only in how long each took; +// if one row failed and ten passed, nothing in the report said which. The +// identity of a row is its parameters, so this asserts that feature, name and +// example together are unique across the whole report -- which is exactly the +// key a consumer needs to be able to use. +func TestOutlineRowsAreDistinguishable(t *testing.T) { + dir := t.TempDir() + t.Setenv(tck.ReportDirEnv, dir) + + tck.Run(t, tck.Config{ + Name: "outline-identity", + Control: plainMemoryControl{}, + NewProvider: func(context.Context) (openfeature.FeatureProvider, error) { + return memprovider.NewInMemoryProvider(tck.CanonicalFlagSet()), nil + }, + Capabilities: []tck.Capability{tck.Object}, + }) + + report := readReport(t, filepath.Join(dir, "outline-identity.json")) + + seen := map[string]bool{} + for _, scenario := range report.Scenarios { + key := scenarioKey(scenario) + if seen[key] { + t.Errorf("two entries share the identity %s; a Scenario Outline row is identified by "+ + "its parameters, so a consumer keying on feature, name and example would keep "+ + "only one of them", key) + } + seen[key] = true + } + + // A report where nothing came from an outline would satisfy the loop above + // while asserting nothing, so the matrix itself is pinned: eleven rows, + // eleven different parameter sets. + const matrix = "Requesting the wrong type returns the code default" + examples := map[string]bool{} + rows := 0 + for _, scenario := range report.Scenarios { + if scenario.Name != matrix { + continue + } + rows++ + if len(scenario.Example) == 0 { + t.Errorf("row %d of %q carries no example, so it is indistinguishable from the others", + rows, matrix) + continue + } + examples[canonicalExample(scenario.Example)] = true + } + if rows == 0 { + t.Fatalf("no entry for %q; either the feature files changed or the suite did not run", matrix) + } + if len(examples) != rows { + t.Errorf("%d rows of %q produced %d distinct examples", rows, matrix, len(examples)) + } + + // The field is present only for outline rows. Emitting an empty object for an + // ordinary scenario would be a second thing for four implementations to agree + // on, and the schema asks for omission instead. + const ordinary = "An unknown flag key returns the code default" + found := false + for _, scenario := range report.Scenarios { + if scenario.Name != ordinary { + continue + } + found = true + if scenario.Example != nil { + t.Errorf("%q is not a Scenario Outline but carries example %v", ordinary, scenario.Example) + } + } + if !found { + t.Errorf("no entry for %q, so the omission of example was not checked", ordinary) + } +} + +// TestSkippedOutlineRowsCarryTheirExample covers the capability gate, which +// records its outcome before the scenario runs and so is a second place the +// example has to be filled in. +// +// A skipped outline row is exactly as ambiguous as a failed one: the four rows +// of the @object outline are four entries that differ in nothing without it. +func TestSkippedOutlineRowsCarryTheirExample(t *testing.T) { + dir := t.TempDir() + t.Setenv(tck.ReportDirEnv, dir) + + tck.Run(t, tck.Config{ + Name: "skipped-outline", + Control: plainMemoryControl{}, + NewProvider: func(context.Context) (openfeature.FeatureProvider, error) { + return memprovider.NewInMemoryProvider(tck.CanonicalFlagSet()), nil + }, + // Empty rather than nil: a nil Capabilities means "declare everything", + // and what this test needs is a provider that declares nothing, so the + // @object outline is gated in the Before hook and never starts. + Capabilities: []tck.Capability{}, + }) + + report := readReport(t, filepath.Join(dir, "skipped-outline.json")) + + const outline = "Requesting a structured flag as a scalar returns the code default" + examples := map[string]bool{} + rows := 0 + for _, scenario := range report.Scenarios { + if scenario.Name != outline { + continue + } + rows++ + if scenario.Outcome != tck.OutcomeNotDeclared { + t.Errorf("%q was reported as %q; @object was not declared", outline, scenario.Outcome) + } + if len(scenario.Example) == 0 { + t.Errorf("a skipped row of %q carries no example, so the report cannot say which row "+ + "was skipped", outline) + continue + } + examples[canonicalExample(scenario.Example)] = true + } + if rows == 0 { + t.Fatalf("no entry for %q; the capability gate did not record it at all", outline) + } + if len(examples) != rows { + t.Errorf("%d skipped rows of %q produced %d distinct examples", rows, outline, len(examples)) + } +} + +// scenarioKey is the identity of a report entry: feature, name and example. +func scenarioKey(scenario tck.ReportScenario) string { + return scenario.Feature + "/" + scenario.Name + "/" + canonicalExample(scenario.Example) +} + +// canonicalExample renders an example so two of them compare equal exactly when +// their parameters do, independently of map iteration order. +func canonicalExample(example map[string]string) string { + keys := make([]string, 0, len(example)) + for key := range example { + keys = append(keys, key) + } + sort.Strings(keys) + + parts := make([]string, 0, len(keys)) + for _, key := range keys { + parts = append(parts, key+"="+example[key]) + } + return strings.Join(parts, "\x00") +} + func readReport(t *testing.T, path string) tck.Report { t.Helper() diff --git a/tools/provider-tck/pkg/tck/run.go b/tools/provider-tck/pkg/tck/run.go index c6e8f591a..196fc4976 100644 --- a/tools/provider-tck/pkg/tck/run.go +++ b/tools/provider-tck/pkg/tck/run.go @@ -68,6 +68,7 @@ func Run(t *testing.T, cfg Config) { }.Run() r.reportSkips() + r.reportExampleGaps() r.writeReport() if status != 0 && !t.Failed() { @@ -89,8 +90,10 @@ type runner struct { // nothing and the alternative is a code path that only ever runs in CI. records []scenarioRecord - // started times scenarios by name. godog gives no scenario-scoped place to - // hang this, and scenarios run serially, so a map keyed by name is enough. + // started times scenarios by pickle id. godog gives no scenario-scoped place + // to hang this, so a map is the only option; the key is the pickle id rather + // than the scenario name because every row of a Scenario Outline shares one + // name and would otherwise share one entry. started map[string]time.Time // gated names the scenarios the capability gate stopped before their first @@ -101,8 +104,18 @@ type runner struct { // and passed. Testing for ErrSkip there silently recorded every skipped // scenario twice, once correctly and once as passed, which is the exact // failure Appendix F forbids. + // + // Keyed by pickle id for a second reason as well as the shared name: Gherkin + // allows an Examples block to carry its own tags, so two rows of one outline + // can differ in whether the gate stops them. Keyed by name, gating one row + // would suppress the after hook for every row, and the rows that did run + // would vanish from the report entirely. gated map[string]bool + // exampleGaps collects outline rows whose Examples row could not be + // resolved. See reportExampleGaps. + exampleGaps []string + // providerName is what the provider called itself, observed from the last // scenario that registered one. providerName string @@ -135,13 +148,13 @@ func (r *runner) beforeScenario(ctx context.Context, sc *godog.Scenario) (contex r.recordSkip(sc.Name, capability) r.recordOutcome(sc, OutcomeNotDeclared, fmt.Sprintf( "requires capability %s, which this provider does not declare", capability.Tag()), 0) - r.markGated(sc.Name) + r.markGated(sc.Id) return ctx, fmt.Errorf( "%w: scenario requires capability %s (Gherkin tag %s), which this provider does not declare. Declared capabilities: %s", godog.ErrSkip, capability, capability.Tag(), formatCapabilities(r.caps.sorted())) } - r.markStarted(sc.Name) + r.markStarted(sc.Id) ctx = withState(ctx, newScenarioState(&r.cfg)) @@ -168,12 +181,12 @@ func (r *runner) afterScenario(ctx context.Context, sc *godog.Scenario, err erro // A capability skip was already recorded before the scenario started. // Recording it again here would put it in the report twice, the second time // as passed. - if !r.wasGated(sc.Name) { + if !r.wasGated(sc.Id) { outcome, reason := OutcomePassed, "" if err != nil { outcome, reason = OutcomeFailed, err.Error() } - r.recordOutcome(sc, outcome, reason, r.elapsed(sc.Name)) + r.recordOutcome(sc, outcome, reason, r.elapsed(sc.Id)) } return ctx, err @@ -186,18 +199,90 @@ func (r *runner) recordOutcome(sc *godog.Scenario, outcome Outcome, reason strin tags = append(tags, tag.Name) } + // Resolved before the lock, because a failure to resolve takes the lock + // itself. + example, order := r.exampleFor(sc) + r.mu.Lock() defer r.mu.Unlock() r.records = append(r.records, scenarioRecord{ - feature: featureName(sc.Uri), - name: sc.Name, - tags: tags, - outcome: outcome, - reason: reason, - duration: duration, + feature: featureName(sc.Uri), + name: sc.Name, + example: example, + exampleOrder: order, + tags: tags, + outcome: outcome, + reason: reason, + duration: duration, }) } +// exampleFor resolves the Examples row a scenario came from, or nil for a +// scenario that is not an outline row. +// +// It is called from both the gate path and the pass/fail path, because a +// skipped outline row is exactly as ambiguous as a failed one: four skipped +// rows of the @object outline are four entries that differ in nothing without +// it. +func (r *runner) exampleFor(sc *godog.Scenario) (map[string]string, int) { + index, err := scenarioExamples() + if err != nil { + r.recordExampleGap(fmt.Sprintf( + "the embedded feature files could not be parsed for their Examples tables: %v", err)) + return nil, 0 + } + + if row, ok := index.rowFor(sc); ok { + return row.values, row.order + } + + if index.isOutline(sc.Uri, sc.Name) { + r.recordExampleGap(fmt.Sprintf( + "scenario %q in %s comes from a Scenario Outline, but no Examples row matched AST node ids %v", + sc.Name, sc.Uri, sc.AstNodeIds)) + } + + return nil, 0 +} + +func (r *runner) recordExampleGap(detail string) { + r.mu.Lock() + defer r.mu.Unlock() + for _, existing := range r.exampleGaps { + if existing == detail { + return + } + } + r.exampleGaps = append(r.exampleGaps, detail) +} + +// reportExampleGaps fails the run if any outline row went unidentified. +// +// Recovering the Examples row depends on reproducing the AST node ids godog +// assigns, which is a coupling to how godog numbers nodes. This is what keeps +// that coupling honest: if a godog release changes the numbering, the report +// would quietly go back to emitting rows that differ in nothing, and a quiet +// return to the ambiguity this field exists to remove is worse than a build +// failure. +func (r *runner) reportExampleGaps() { + r.mu.Lock() + gaps := make([]string, len(r.exampleGaps)) + copy(gaps, r.exampleGaps) + r.mu.Unlock() + + if len(gaps) == 0 { + return + } + + message := []string{fmt.Sprintf( + "provider-tck [%s]: %d Scenario Outline row(s) could not be identified by their Examples parameters, "+ + "so the report cannot distinguish them:", r.cfg.Name, len(gaps))} + for _, gap := range gaps { + message = append(message, " - "+gap) + } + r.t.Error(strings.Join(message, "\n")) +} + // markGated and markStarted create their maps on first use. // // The runner has to work as a zero value: it is constructed as a struct literal From 6290b99543f73392b097813ffed73b2a52114fec Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 24 Aug 2026 21:40:41 +0200 Subject: [PATCH 4/5] fix(provider-tck): count a capability as exercised only when a scenario ran A capability whose scenarios were all skipped still reported as passed. The in-memory report claimed "@events": "passed" while both scenarios in events.feature had been skipped. Those scenarios carry @events alongside @stale and @configuration-change, so withholding either one skipped them -- and the rollup counted a skipped scenario's remaining tags as exercising their capabilities. @events was declared, so it missed the not-declared branch, and fell through to passed on the strength of two scenarios that never executed. It is the same vacuous pass as the reserved-capability case fixed alongside it, reached by a different route: the rollup was counting tag presence rather than execution. A scenario now contributes to its capabilities only when its outcome is passed or failed. A declared capability whose every scenario was skipped falls to zero exercised and is omitted, which is the honest answer -- the suite has the question but never got to put it to this provider. Found by the Python implementation, which reached the same rollup semantics and then noticed what they produced. Signed-off-by: Simon Schrottner --- tools/provider-tck/pkg/tck/report.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tools/provider-tck/pkg/tck/report.go b/tools/provider-tck/pkg/tck/report.go index 52f5bbe0e..dcb888849 100644 --- a/tools/provider-tck/pkg/tck/report.go +++ b/tools/provider-tck/pkg/tck/report.go @@ -172,6 +172,16 @@ func (r *runner) buildReport() Report { Reason: rec.reason, DurationMs: float64(rec.duration.Microseconds()) / 1000.0, }) + // Only a scenario that actually ran exercises anything. A scenario + // skipped for one undeclared capability still carries its other tags, + // and counting those would report a capability as passed on the + // strength of a scenario that never executed: events.feature's + // scenarios carry @events alongside @stale and @configuration-change, + // so withholding either left @events reading "passed" while both of its + // scenarios were skipped. + if rec.outcome != OutcomePassed && rec.outcome != OutcomeFailed { + continue + } for _, tag := range rec.tags { capability, gates := CapabilityForTag(tag) if !gates { From be2736140242518ae30fd12fae3031ccffc630e8 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 24 Aug 2026 21:48:21 +0200 Subject: [PATCH 5/5] docs(provider-tck): record that the suite runs against a caching provider @caching is reserved and no scenario carries it, so the known-gaps list said caching was not covered and left it there. That understates the situation. flagd's RPC resolver enables an LRU cache by default and rewrites the reason to CACHED on a hit, and the adoption does not turn it off. So the suite already runs against a caching provider while asserting STATIC everywhere, and passes only because no scenario evaluates the same flag twice in a way that hits the cache. A scenario added later that does will fail against flagd RPC with CACHED, and the failure will look like a provider defect rather than a test-design one. The configuration-change scenario is also already leaning on cache invalidation without saying so: against flagd RPC it reads changing-flag, changes it, and reads again, which only produces the right answer because the change event evicts the entry. A provider whose cache was not invalidated would serve the stale value and fail a scenario that never mentions caching. Writing @caching scenarios is deliberately deferred. Recording the trap is not, because the cost of rediscovering it falls on whoever adds the next scenario. Signed-off-by: Simon Schrottner --- tools/provider-tck/README.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tools/provider-tck/README.md b/tools/provider-tck/README.md index b90b693db..9a2ea60cd 100644 --- a/tools/provider-tck/README.md +++ b/tools/provider-tck/README.md @@ -352,7 +352,16 @@ the SDK's provider rather than reimplementing it — every resolution decision i then a provider that silently drops the context passes. `@targeting` is reserved for these. - **`POST /restart` is unused.** No current scenario needs a bounded outage — the stale scenario uses an explicit disconnect and reconnect — so `tck.ConnectionControl` has no `DisconnectFor`. -- **Caching, hooks and flag metadata** are not covered. +- **Hooks and flag metadata** are not covered. +- **Caching is not covered, and the suite is quietly exposed to it.** `@caching` is reserved and no + scenario carries it, but flagd's RPC resolver enables an LRU cache *by default* and rewrites the + reason to `CACHED` on a hit. The adoption does not turn it off, so the suite already runs against a + caching provider while asserting `STATIC` everywhere. It passes only because no scenario evaluates + the same flag twice in a way that hits the cache — so a scenario added later that does will fail + against flagd RPC with `CACHED`, and the failure will look like a provider defect rather than a + test-design one. Note also that the configuration-change scenario already depends on cache + invalidation working without saying so: against flagd RPC it reads `changing-flag`, changes it, and + reads again, which only gives the right answer because the change event evicts the entry. [appendix-a]: https://github.com/open-feature/spec/blob/main/specification/appendix-a-included-utilities.md [appendix-b]: https://github.com/open-feature/spec/blob/main/specification/appendix-b-gherkin-suites.md