diff --git a/.golangci.yaml b/.golangci.yaml index cb16585..143e526 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -60,7 +60,7 @@ linters: cyclop: max-complexity: 15 funlen: - lines: 80 + lines: 90 gosec: excludes: - G304 diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ee43d2..af68e29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,25 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Changed + +- Panic failure messages now name the phase that panicked (test, sub-test, or the specific suite/plugin hook). +- Panics in `BeforeAll` & `AfterAll` hooks (both suite and plugin) are now caught and recorded as test failures; a panic in `BeforeAll` still prevents the suite tests from running. +- `t.Setenv` and `t.Chdir` now enforce the standard library's conflict guards against `t.Parallel`. +- The error for a non-pointer exported field now names the owning type and field. +- Better wording for a warning when `CasesXxx` returns empty slice. + +### Fixed + +- Panics in tests now record the actual recovered value, are observed by after-hooks as a failed test. +- Plugin hooks and overrides with equal priority now run deterministically, in the declaration order of plugins. +- Overrides are now applied in the documented priority order, consistent with hooks. +- `t.Error` called after `t.Fatal` (e.g. from a cleanup) no longer downgrades the recorded fatal failure kind. +- Annotations no longer leak between instantiations of a generic suite. +- Persistent cache reads no longer follow a symlink swapped in after validation. + ## [1.6.0] - 2026-07-17 ### Added diff --git a/annotations.go b/annotations.go index 10868b8..e2af428 100644 --- a/annotations.go +++ b/annotations.go @@ -86,5 +86,23 @@ func getID[Suite any](test reflect.Value) testID { name = strings.ReplaceAll(name, "(*"+suiteName+")", suiteName) - return testID(name) + // Runtime erases generic type arguments to "[...]" in function names: + // + // KVSuite[int].TestGet -> "pkg.KVSuite[...].TestGet" + // KVSuite[string].TestGet -> "pkg.KVSuite[...].TestGet" (same!) + // + // while suiteName keeps them ("KVSuite[int]"). + // + // Code below gets us: + // + // KVSuite[int]|pkg.KVSuite[...].TestGet + // KVSuite[string]|pkg.KVSuite[...].TestGet + + if base, _, isGeneric := strings.Cut(suiteName, "["); isGeneric { + erased := base + "[...]" + + name = strings.ReplaceAll(name, "(*"+erased+")", erased) + } + + return testID(suiteName + "|" + name) } diff --git a/collector.go b/collector.go index 48dce28..ae2688a 100644 --- a/collector.go +++ b/collector.go @@ -392,7 +392,7 @@ func (tc *testsCollector[Suite, T]) newParametrizedTest( warnf( tb, - "(%[1]s).Cases%[2]s provides zero values, (%[1]s).%[3]s won't run", + "(%[1]s).Cases%[2]s returned empty slice, (%[1]s).%[3]s will not run", structName, caseName, method.Name, diff --git a/construct.go b/construct.go index 5714b45..261210f 100644 --- a/construct.go +++ b/construct.go @@ -3,6 +3,7 @@ package testo import ( "fmt" "reflect" + "slices" "github.com/ozontech/testo/internal/reflectutil" "github.com/ozontech/testo/internal/stack" @@ -61,66 +62,48 @@ func construct[T CommonT]( )) } - //nolint:nestif // TODO: factor out common logic - if parent == nil { - pluginTypes := make(map[reflect.Type]struct{}) - - collectPlugins(realType, pluginTypes) - - delete(pluginTypes, realType) - - plugins := make(map[reflect.Type]testoplugin.Plugin, len(pluginTypes)) - - for pluginType := range pluginTypes { - var child testoplugin.Plugin - - if pluginType == reflect.TypeFor[*testoT]() { - child = &seed - } else { - v := reflect.New(pluginType.Elem()) - - reflectutil.Fill(v) - - child = v.Interface().(testoplugin.Plugin) - } - - plugins[pluginType] = child - } - - seed.plugins = plugins + if parent != nil { + seed.pluginOrder = (*parent).unwrap().pluginOrder } else { - parentUnwrapped := (*parent).unwrap() + seen := make(map[reflect.Type]struct{}) - plugins := make(map[reflect.Type]testoplugin.Plugin, len(parentUnwrapped.plugins)) + collectPlugins(realType, seen, &seed.pluginOrder) - for pluginType := range parentUnwrapped.plugins { - var child testoplugin.Plugin + // the T type itself is not a plugin on its own. + seed.pluginOrder = slices.DeleteFunc(seed.pluginOrder, func(typ reflect.Type) bool { + return typ == realType + }) + } - if pluginType == reflect.TypeFor[*testoT]() { - child = &seed - } else { - v := reflect.New(pluginType.Elem()) + plugins := make(map[reflect.Type]testoplugin.Plugin, len(seed.pluginOrder)) - reflectutil.Fill(v) + for _, pluginType := range seed.pluginOrder { + var child testoplugin.Plugin - child = v.Interface().(testoplugin.Plugin) - } + if pluginType == reflect.TypeFor[*testoT]() { + child = &seed + } else { + v := reflect.New(pluginType.Elem()) - plugins[pluginType] = child + reflectutil.Fill(v) + + child = v.Interface().(testoplugin.Plugin) } - seed.plugins = plugins + plugins[pluginType] = child } + seed.plugins = plugins + specsStack := stack.New[typedPlugin]() - for pluginType, pluginValue := range seed.plugins { + for _, pluginType := range seed.pluginOrder { specsStack.Push(typedPlugin{ - Plugin: pluginValue, + Plugin: seed.plugins[pluginType], Type: pluginType, }) - setPlugins(reflect.ValueOf(pluginValue), seed.plugins, &specsStack) + setPlugins(reflect.ValueOf(seed.plugins[pluginType]), seed.plugins, &specsStack) } setPlugins(value, seed.plugins, &specsStack) @@ -137,6 +120,10 @@ func construct[T CommonT]( continue } + // For top-level tests the parent is a typed-nil instance of the + // plugin's own type: the interface is non-nil, so unconditional + // parent.(*MyPlugin) assertions keep working, and the concrete + // pointer is nil. See [testoplugin.Plugin]. var parentPlugin testoplugin.Plugin if parent != nil { @@ -148,7 +135,17 @@ func construct[T CommonT]( specs[p.Type] = p.Plugin.Plugin(parentPlugin, seed.options()...) } - seed.spec = mergeSpecs(t, mapValues(specs)...) + // merge specs in the declaration order of plugins inside T so that + // equal-priority hooks and overrides run deterministically. + orderedSpecs := make([]testoplugin.Spec, 0, len(specs)) + + for _, pluginType := range seed.pluginOrder { + if spec, ok := specs[pluginType]; ok { + orderedSpecs = append(orderedSpecs, spec) + } + } + + seed.spec = mergeSpecs(t, orderedSpecs...) return value.Elem().Interface().(T) } @@ -205,7 +202,9 @@ func setPlugins( if field.Kind() != reflect.Pointer { panic( fmt.Sprintf( - "testo: all exported fields in T must be pointers, got: %s", + "testo: all exported fields in T and plugins must be pointers: field %s.%s has non-pointer type %s", + v.Type(), + v.Type().Field(i).Name, field.Type(), ), ) @@ -217,9 +216,15 @@ func setPlugins( var pluginInterfaceType = reflect.TypeFor[testoplugin.Plugin]() -func collectPlugins(typ reflect.Type, plugins map[reflect.Type]struct{}) { +// collectPlugins gathers plugin types in their declaration order: +// a depth-first walk over the exported fields of typ. +func collectPlugins(typ reflect.Type, seen map[reflect.Type]struct{}, order *[]reflect.Type) { if typ.Implements(pluginInterfaceType) { - plugins[typ] = struct{}{} + if _, ok := seen[typ]; !ok { + seen[typ] = struct{}{} + + *order = append(*order, typ) + } } typ = reflectutil.Elem(typ) @@ -232,17 +237,7 @@ func collectPlugins(typ reflect.Type, plugins map[reflect.Type]struct{}) { field := typ.Field(i) if field.IsExported() { - collectPlugins(field.Type, plugins) + collectPlugins(field.Type, seen, order) } } } - -func mapValues[K comparable, V any](m map[K]V) []V { - values := make([]V, 0, len(m)) - - for _, v := range m { - values = append(values, v) - } - - return values -} diff --git a/construct_test.go b/construct_test.go index 12fcc93..e6f44c5 100644 --- a/construct_test.go +++ b/construct_test.go @@ -29,16 +29,25 @@ func (m *MockPluginWithT) Plugin(testoplugin.Plugin, ...testoplugin.Option) test } type MockPluginWithoutT struct { - parent *MockPluginWithoutT - options []testoplugin.Option - initCalled int + parent *MockPluginWithoutT + parentNonNil bool + options []testoplugin.Option + initCalled int } func (m *MockPluginWithoutT) Plugin( parent testoplugin.Plugin, options ...testoplugin.Option, ) testoplugin.Spec { - m.parent = parent.(*MockPluginWithoutT) + // For top-level tests parent is a typed-nil *MockPluginWithoutT inside a + // non-nil interface, as documented in [testoplugin.Plugin]. Recording the + // interface comparison separately from the asserted pointer lets + // TestConstruct distinguish typed-nil from a true nil interface. + m.parentNonNil = parent != nil + if parent != nil { + m.parent = parent.(*MockPluginWithoutT) + } + m.options = options m.initCalled++ @@ -122,6 +131,23 @@ func TestConstruct(t *testing.T) { } } + // Top level: parent must be a typed-nil instance of the plugin type - + // a non-nil interface wrapping a nil pointer - so that plugins doing + // an unconditional parent.(*MyPlugin) assertion keep working. + if !res.MockPluginWithoutT.parentNonNil { + t.Error( + "res.MockPluginWithoutT: top-level parent interface is nil, want typed-nil instance", + ) + } + + if res.MockPluginWithoutT.parent != nil { + t.Error("res.MockPluginWithoutT.parent is not a nil pointer for a top-level test") + } + + if child.MockPluginWithoutT.parent != res.MockPluginWithoutT { + t.Error("child.MockPluginWithoutT.parent does not point to the parent's plugin") + } + if !reflect.DeepEqual(res.T, child.T.parent) { t.Error("res.T not equal to child.T.parent") } @@ -167,6 +193,128 @@ func TestConstruct(t *testing.T) { }) } +// nestedPluginCalls records the order in which Plugin is called for the mock +// plugins below. Only TestConstructNestedPluginOrder and its plugins touch it. +var nestedPluginCalls []string + +type MockInnerPlugin struct{ initCalled int } + +func (m *MockInnerPlugin) Plugin(testoplugin.Plugin, ...testoplugin.Option) testoplugin.Spec { + m.initCalled++ + nestedPluginCalls = append(nestedPluginCalls, "Inner") + + return testoplugin.Spec{} +} + +type MockOuterPlugin struct { + Inner *MockInnerPlugin + + initCalled int +} + +func (m *MockOuterPlugin) Plugin(testoplugin.Plugin, ...testoplugin.Option) testoplugin.Spec { + m.initCalled++ + nestedPluginCalls = append(nestedPluginCalls, "Outer") + + return testoplugin.Spec{} +} + +type MockDeepPlugin struct{ initCalled int } + +func (m *MockDeepPlugin) Plugin(testoplugin.Plugin, ...testoplugin.Option) testoplugin.Spec { + m.initCalled++ + nestedPluginCalls = append(nestedPluginCalls, "Deep") + + return testoplugin.Spec{} +} + +type MockMidPlugin struct { + Deep *MockDeepPlugin + + initCalled int +} + +func (m *MockMidPlugin) Plugin(testoplugin.Plugin, ...testoplugin.Option) testoplugin.Spec { + m.initCalled++ + nestedPluginCalls = append(nestedPluginCalls, "Mid") + + return testoplugin.Spec{} +} + +type NestedMockT struct { + *T + + Outer *MockOuterPlugin + Mid *MockMidPlugin +} + +func TestConstructNestedPluginOrder(t *testing.T) { + t.Parallel() + + nestedPluginCalls = nil + + res := construct[NestedMockT](t, nil, nil) + + if res.Outer.Inner == nil { + t.Fatal("res.Outer.Inner is nil") + } + + if res.Mid.Deep == nil { + t.Fatal("res.Mid.Deep is nil") + } + + for _, counter := range []struct { + name string + value int + }{ + {"Outer", res.Outer.initCalled}, + {"Inner", res.Outer.Inner.initCalled}, + {"Mid", res.Mid.initCalled}, + {"Deep", res.Mid.Deep.initCalled}, + } { + if counter.value != 1 { + t.Errorf("%s: initCalled = %d, want 1", counter.name, counter.value) + } + } + + // Plugin must be called on a nested plugin before its container, so that + // the container observes fully initialized nested plugins. Sibling + // subtrees have no guaranteed relative order, so only pairwise + // nested-before-container positions are asserted. + for _, pair := range []struct{ nested, container string }{ + {"Inner", "Outer"}, + {"Deep", "Mid"}, + } { + nestedIdx := slices.Index(nestedPluginCalls, pair.nested) + containerIdx := slices.Index(nestedPluginCalls, pair.container) + + if nestedIdx < 0 || containerIdx < 0 { + t.Fatalf("missing Plugin calls in %v", nestedPluginCalls) + } + + if nestedIdx > containerIdx { + t.Errorf( + "Plugin called for container %s before nested %s: %v", + pair.container, pair.nested, nestedPluginCalls, + ) + } + } + + // pluginOrder drives spec merging and must stay in declaration order: + // containers before their nested plugins, siblings as declared in T. + wantOrder := []reflect.Type{ + reflect.TypeFor[*T](), + reflect.TypeFor[*MockOuterPlugin](), + reflect.TypeFor[*MockInnerPlugin](), + reflect.TypeFor[*MockMidPlugin](), + reflect.TypeFor[*MockDeepPlugin](), + } + + if !slices.Equal(wantOrder, res.unwrap().pluginOrder) { + t.Errorf("pluginOrder = %v, want %v", res.unwrap().pluginOrder, wantOrder) + } +} + func mustPanic(t *testing.T, f func()) { t.Helper() diff --git a/docs/how-to.md b/docs/how-to.md index 2ba858d..44977cb 100644 --- a/docs/how-to.md +++ b/docs/how-to.md @@ -44,7 +44,7 @@ If for at least one required parameter function `CasesXxx` returns zero values Testo will log a warning with similar message: ```txt -main_test.go:15: testo: (*main.Suite).CasesName provides zero values, (*main.Suite).TestFoo won't run +main_test.go:15: testo: (*main.Suite).CasesName returned empty slice, (*main.Suite).TestFoo will not run ``` To turn this log into fatal error and do not proceed with further execution diff --git a/docs/technical-overview.md b/docs/technical-overview.md index 904164c..836aff3 100644 --- a/docs/technical-overview.md +++ b/docs/technical-overview.md @@ -10,7 +10,8 @@ When you call `testo.RunSuite` the following happens: A root test named the same as a suite is run { Suite tests are collected and verified. - Plugins are collected and initialized with ".Plugin(parent: nil, options)" method call, if implemented. Innermost plugins are initialized first. + Plugins are collected and initialized with ".Plugin(parent, options)" method call, if implemented. Innermost plugins are initialized first. + At this top level, parent is a typed-nil instance of the plugin's own type: the interface is non-nil, but the concrete pointer inside is nil. "BeforeAll" plugin hooks are called. "BeforeAll" suite hook is called. @@ -52,12 +53,12 @@ A root test named the same as a suite is run { Testo **will** catch panics from tests, including `BeforeEach`, `BeforeEachSub`, `AfterEachSub` & `AfterEach` hooks. Other tests will run even if some tests are panicking. -Testo **will not** catch panics from `BeforeAll` & `AfterAll`. +Testo **will** catch panics from `BeforeAll` & `AfterAll`. Panic in these hooks will result in suite tests not running. ## Plugins -Testo uses dependency-injection-like mechanism to enable cross-plugin commuincation. +Testo uses dependency-injection-like mechanism to enable cross-plugin communication. For example, assume we have plugin `X` and plugin `Y`. Plugin `X` needs to interact with plugin `Y`. To make it possible, `X` needs to embed `Y`: diff --git a/examples/06_errors/main_test.go b/examples/06_errors/main_test.go index 5f2ef30..35d15e4 100644 --- a/examples/06_errors/main_test.go +++ b/examples/06_errors/main_test.go @@ -38,7 +38,7 @@ type EmptyCases struct{ testo.Suite[T] } func (EmptyCases) CasesFoo() []int { return nil } -// testo: warning: (*main.EmptyCases).CasesFoo provides zero values, (*main.EmptyCases).Test won't run +// testo: warning: (*main.EmptyCases).CasesFoo returned empty slice, (*main.EmptyCases).Test will not run func (EmptyCases) Test(t T, p struct{ Foo int }) {} type WrongT struct{ testo.Suite[T] } diff --git a/examples/06_errors/output.golden b/examples/06_errors/output.golden index c76f382..e8fc325 100644 --- a/examples/06_errors/output.golden +++ b/examples/06_errors/output.golden @@ -13,7 +13,7 @@ main_test.go:56: testo: wrong param signature for (*main.InvalidCases).Test: (*main.InvalidCases).CasesFoo provides string values, not assignable to param "Foo" of type int === RUN Test/empty_cases === RUN Test/empty_cases/EmptyCases - main_test.go:57: testo: warning: (*main.EmptyCases).CasesFoo provides zero values, (*main.EmptyCases).Test won't run + main_test.go:57: testo: warning: (*main.EmptyCases).CasesFoo returned empty slice, (*main.EmptyCases).Test will not run === RUN Test/empty_cases/EmptyCases/testo! === RUN Test/wrong_t === RUN Test/wrong_t/WrongT diff --git a/plugin.go b/plugin.go index ba02540..b1cd1d2 100644 --- a/plugin.go +++ b/plugin.go @@ -226,7 +226,7 @@ func mergeOverride[F any]( getter func(testoplugin.Overrides) testoplugin.Override[F], ) func(F) F { return func(f F) F { - for _, o := range overrides { + for _, o := range slices.Backward(overrides) { if override := getter(o); override != nil { f = override(f) } diff --git a/runner.go b/runner.go index dd04c50..cf824b3 100644 --- a/runner.go +++ b/runner.go @@ -193,26 +193,15 @@ func Run[T CommonT]( options..., ) - defer func() { - if r := recover(); r != nil { - trace := string(debug.Stack()) - - t.unwrap().reflection.Modify(func(r *testoreflect.Reflection) { - r.Panic = &testoreflect.PanicInfo{ - Value: r, - Trace: trace, - } - }) - - t.Fatalf("testo: test %q panicked: %v\n\n%s", t.Name(), r, trace) - } - }() + defer runProtectedHook(t, "plugin AfterEachSub hook", t.unwrap().spec.Hooks.AfterEachSub) - defer runHook(t, t.unwrap().spec.Hooks.AfterEachSub) + runProtectedHook(t, "plugin BeforeEachSub hook", t.unwrap().spec.Hooks.BeforeEachSub) - runHook(t, t.unwrap().spec.Hooks.BeforeEachSub) + runProtected(t, "sub-test", func() { + t.Helper() - f(t) + f(t) + }) }) } @@ -306,20 +295,36 @@ func (r *runner[Suite, T]) runSuiteTests(t T, s Suite, tests suiteTests[Suite, T t.Helper() defer func() { - if !t.Skipped() { - runHook(t, t.unwrap().spec.Hooks.AfterAll) + t.Helper() + + if t.Skipped() { + return } + + runProtectedHook(t, "plugin AfterAll hook", t.unwrap().spec.Hooks.AfterAll) }() - runHook(t, t.unwrap().spec.Hooks.BeforeAll) + runProtectedHook(t, "plugin BeforeAll hook", t.unwrap().spec.Hooks.BeforeAll) defer func() { - if !t.Skipped() { - s.AfterAll(t) + t.Helper() + + if t.Skipped() { + return } + + runProtected(t, "suite AfterAll hook", func() { + t.Helper() + + s.AfterAll(t) + }) }() - s.BeforeAll(t) + runProtected(t, "suite BeforeAll hook", func() { + t.Helper() + + s.BeforeAll(t) + }) suiteReflection := t.unwrap().reflection.Load().Suite @@ -382,30 +387,73 @@ func (r *runner[Suite, T]) runSuiteTest( ) { t.Helper() + defer runProtectedHook(t, "plugin AfterEach hook", t.unwrap().spec.Hooks.AfterEach) + + runProtectedHook(t, "plugin BeforeEach hook", t.unwrap().spec.Hooks.BeforeEach) + + defer runProtected(t, "suite AfterEach hook", func() { + t.Helper() + + s.AfterEach(t) + }) + + runProtected(t, "suite BeforeEach hook", func() { + t.Helper() + + s.BeforeEach(t) + }) + + runProtected(t, "test", func() { + t.Helper() + + test.Run(s, t) + }) +} + +// runProtected runs f, converting a panic into a recorded test failure +// before deferred after-hooks run, so that they observe the failed state. +func runProtected[T CommonT](t T, kind string, f func()) { + t.Helper() + defer func() { - if r := recover(); r != nil { - trace := string(debug.Stack()) - - t.unwrap().reflection.Modify(func(ref *testoreflect.Reflection) { - ref.Panic = &testoreflect.PanicInfo{ - Value: r, - Trace: trace, - } - }) + t.Helper() - t.Fatalf("testo: test %q panicked: %v\n\n%s", t.Name(), r, trace) + if rec := recover(); rec != nil { + recordPanic(t, kind, rec) } }() - defer runHook(t, t.unwrap().spec.Hooks.AfterEach) + f() +} - runHook(t, t.unwrap().spec.Hooks.BeforeEach) +func runProtectedHook[T CommonT](t T, kind string, hook testoplugin.Hook) { + t.Helper() + + runProtected(t, kind, func() { + t.Helper() - defer s.AfterEach(t) + runHook(t, hook) + }) +} - s.BeforeEach(t) +// recordPanic stores rec as the test's panic information and fails the test. +// An already recorded panic is kept, so the original panic value survives +// even if an after-hook panics during unwinding. +func recordPanic[T CommonT](t T, kind string, rec any) { + t.Helper() + + trace := string(debug.Stack()) + + t.unwrap().reflection.Modify(func(ref *testoreflect.Reflection) { + if ref.Panic == nil { + ref.Panic = &testoreflect.PanicInfo{ + Value: rec, + Trace: trace, + } + } + }) - test.Run(s, t) + t.Fatalf("testo: %s in %q panicked: %v\n\n%s", kind, t.Name(), rec, trace) } func (r *runner[Suite, T]) applyPlan( diff --git a/t.go b/t.go index dabaf09..24f7097 100644 --- a/t.go +++ b/t.go @@ -81,6 +81,12 @@ type ( failureKind atomicInt[testoreflect.TestFailureKind] hasFatalSubtest atomic.Bool + // isParallel and denyParallel mirror the testing package guards: + // Setenv and Chdir mutate process-wide state and therefore conflict + // with Parallel. See [T.checkParallel]. + isParallel atomic.Bool + denyParallel atomic.Bool + // propagateParallel if enabled, will route .Parallel() calls // to suite's TestingT. // @@ -88,6 +94,11 @@ type ( propagateParallel bool plugins map[reflect.Type]testoplugin.Plugin + + // pluginOrder holds plugin types in their declaration order inside + // the user T struct. Specs are merged in this order so that + // equal-priority hooks and overrides run deterministically. + pluginOrder []reflect.Type } testoT = T @@ -124,6 +135,12 @@ func (t *T) Parallel() { func (t *T) parallel() { t.Helper() + if t.denyParallel.Load() { + panic("testo: test using t.Setenv or t.Chdir can not use t.Parallel") + } + + t.isParallel.Store(true) + if t.propagateParallel { t.reflection.Load().Suite.TestingT.Parallel() @@ -133,6 +150,24 @@ func (t *T) parallel() { t.common.Parallel() } +// checkParallel mirrors testing.common.checkParallel: methods that mutate +// process-wide state (Setenv, Chdir) cannot be used in parallel tests or +// tests with parallel ancestors, and forbid a later t.Parallel call. +func (t *T) checkParallel(method string) { + t.Helper() + + for c := t; c != nil; c = c.parent { + if c.isParallel.Load() { + panic( + "testo: t." + method + " called after t.Parallel; " + + "it cannot be used in parallel tests or tests with parallel ancestors", + ) + } + } + + t.denyParallel.Store(true) +} + // Setenv calls os.Setenv(key, value) and uses Cleanup to // restore the environment variable to its original value // after the test. @@ -151,6 +186,8 @@ func (t *T) Setenv(key, value string) { func (t *T) setenv(key, value string) { t.Helper() + t.checkParallel("Setenv") + prevValue, ok := os.LookupEnv(key) if err := os.Setenv(key, value); err != nil { @@ -352,6 +389,8 @@ func (t *T) Chdir(dir string) { func (t *T) chdir(dir string) { t.Helper() + t.checkParallel("Chdir") + oldwd, err := os.Open(".") if err != nil { t.Fatal(err) @@ -443,7 +482,13 @@ func (t *T) markFailure(kind testoreflect.TestFailureKind) { return } - t.failureKind.Store(kind) + // never downgrade an already recorded fatal failure - e.g. t.Error + // called from a cleanup after t.Fatal must keep the fatal kind. + if !t.failureKind.CompareAndSwap(testoreflect.TestFailureKindNone, kind) && + kind == testoreflect.TestFailureKindFatal { + t.failureKind.Store(kind) + } + t.failureSource.Store(testoreflect.TestFailureSourceSelf) parent := t.parent diff --git a/t_test.go b/t_test.go index 7a46588..07a6371 100644 --- a/t_test.go +++ b/t_test.go @@ -90,7 +90,7 @@ type TSuiteT struct { type TSuitePlugin struct{} -var tSuiteOverridesCalls struct { +type tSuiteCallCounts struct { log, parallel, setenv, @@ -109,6 +109,8 @@ var tSuiteOverridesCalls struct { cleanup int } +var tSuiteOverridesCalls tSuiteCallCounts + func (p *TSuitePlugin) Plugin(testoplugin.Plugin, ...testoplugin.Option) testoplugin.Spec { return testoplugin.Spec{ Overrides: testoplugin.Overrides{ @@ -234,6 +236,9 @@ func (TSuite) Test(t TSuiteT) { } func TestSuiteT(t *testing.T) { + // reset the counters so the test stays correct under -count>1 + tSuiteOverridesCalls = tSuiteCallCounts{} + t.Parallel() if !RunSuite(t, new(TSuite)) { diff --git a/testocache/cache.go b/testocache/cache.go index 63c21cb..ba84226 100644 --- a/testocache/cache.go +++ b/testocache/cache.go @@ -312,7 +312,7 @@ func (c Cache) dir() (string, error) { } func readEntry(p, key string) ([]byte, error) { - info, err := os.Lstat(p) + linfo, err := os.Lstat(p) if err != nil { if errors.Is(err, os.ErrNotExist) { return nil, ErrNotFound @@ -321,11 +321,11 @@ func readEntry(p, key string) ([]byte, error) { return nil, err } - if !info.Mode().IsRegular() { + if !linfo.Mode().IsRegular() { return nil, ErrNotFound } - value, err := os.ReadFile(p) + f, err := os.Open(p) if err != nil { if errors.Is(err, os.ErrNotExist) { return nil, ErrNotFound @@ -333,6 +333,23 @@ func readEntry(p, key string) ([]byte, error) { return nil, err } + defer f.Close() + + // Stat the opened descriptor and require identity with the Lstat result, + // so a symlink swapped in between the two calls cannot be followed. + finfo, err := f.Stat() + if err != nil { + return nil, err + } + + if !finfo.Mode().IsRegular() || !os.SameFile(linfo, finfo) { + return nil, ErrNotFound + } + + value, err := io.ReadAll(f) + if err != nil { + return nil, err + } storedKey, value, ok := bytes.Cut(value, []byte{0}) if !ok || string(storedKey) != key { diff --git a/testoplugin/plugin.go b/testoplugin/plugin.go index e391ad8..c9b15b0 100644 --- a/testoplugin/plugin.go +++ b/testoplugin/plugin.go @@ -5,7 +5,16 @@ // Plugins can implement [Plugin] interface to be registered as such. // // Method "Plugin" will be called for each plugin before running a suite. -// Parent is nil for top-level tests. For sub-tests it referes to the plugin instance of the parent test. +// For sub-tests, parent refers to the plugin instance of the parent test. +// For top-level tests, parent is a typed-nil instance of the plugin's own +// type: the interface itself is non-nil, but the concrete pointer is nil, +// so an unconditional type assertion like parent.(*PluginFoo) is safe. +// To detect the top level, nil-check the asserted pointer, not the interface: +// +// prev, _ := parent.(*PluginFoo) +// if prev != nil { +// // sub-test: use prev +// } // // It is encouraged to ensure a plugin implements [Plugin] interface with the following line: //