Implement generic transformer decorator: ginkgo.ContextTransformer

[advdv]

This feature has been discussed in the following issue: #892

In short: it is common for a set of test case to be dependant on a certain value being part of the context.Context. For example when testing HTTP handlers. Each case might require an ID token in the context.Context that identifies the principal of the request. Currently there is not really a good way to share such a testing setup between (nested) test cases.

A way to hook into Ginkgo unilaterally to inject these values might be desired. As an example, this is what we do now (notice the "IDed" wrappers we need to include everywhere):

var _ = Describe("user", func() {
	var mdl model.Model
	var tx pgx.Tx

	BeforeEach(tests.EnsureIsolatedTx(&tx))
	BeforeEach(tests.EnsureModel(fx.Populate(&mdl)))

	It("should setup model", tests.IDed(func(ctx context.Context) {

	}))

	It("should create and list users", tests.IDed(func(ctx context.Context) {

	}))

	It("should create and list organizations", tests.IDed(func(ctx context.Context) {

	}))
})

It would be nice if we could do something like this:

var _ = Describe("user", tests.IDed, func() {
	var mdl model.Model
	var tx pgx.Tx

	BeforeEach(tests.EnsureIsolatedTx(&tx))
	BeforeEach(tests.EnsureModel(fx.Populate(&mdl)))

	It("should setup model", func(ctx context.Context) {

	})

	It("should create and list users", func(ctx context.Context) {

	})

	It("should create and list organizations", func(ctx context.Context) {

	})
})

@onsi proposed the following idea:

I could imagine a generic transformer decorator of type ginkgo.ContextTransformer with signature func (ctx context.Context) ctx.Context. Any runnable node that accepts a context (which would include setup nodes and cleanup nodes) would receive a context that is first passed through that decorator. I could imagine some edge cases and would want to make sure the current lifecycle of the context object makes sense with what you're proposing.

[onsi]

hey I'm thinking about implementing this next. if a node is passed a new ContextWrapper decorator:

ContextWrapper(func(ctx context.Context) context.Context)

then any child nodes that accept a context.Context will receive a context that is first passed through the ContextWrapper. These can be chained with outermost wrappers running first.

There are a few caveats that might confuse some users so would appreciate thoughts on these:

1. Each node always gets its own SpecContext - there is not (yet) a single context that spans the entire spec. So changes made in one node (eg a BeforeEach) will not carry over to future nodes.
2. The user should always wrap the provided context.Context (which will be a Ginkgo SpecContext) I'm considering throwing an error if they fail to do that and, instead, return a different context.
3. Every node that accepts a context.Context will invoke the wrapper. In the case you describe, if the functions passed to BeforeEach accept a context that context will go through the wrapper. Would this be ok? or surprising?
4. The wrappers would run as part of the node (i.e. failures would be associated with the node, with the specific line number pointing to the line in the wrapper that failed). If the wrapper is defined on a parent container, however, then it will not have access to the closure variables within the container. I imagine this is fine and easy to work around but wanted to call it out as an aspect to this what won't be super ergonomic.

Given all those pieces - does this still work for your usecase?

[advdv]

Cool! these are some interesting thoughts. I'm not that deep into the codebase but:

1. For me that would not be confusing, I've been learned to isolate each node pretty well and only user shared state with a shared value that is set in the BeforeEach. It COULD maybe be made more clear if the signature becomes ContextWrapper(func(ctx SpecContext) context.Context) but this makes it less flexible
2. Right, that makes sense.
3. Hmmm, tough question. I'm not sure if I can fully comprehend the rectifications of this. In general I think the leaf nodes are usually the ones that assert something depending on the context. I can't REALLY think of a decorator that would cause something surprising in a beforeEach. BUT: contexts are usually not typed very well, and may trigger behaviour that is hard to debug. Contexts get send around to all sorts of places and if any place just happens to assert some value in the context that does weird stuff it might be very confusing.

Sooo, maybe that means it is better to only allow context wrapping to effect leaf nodes. If it turns out people would need it in BeforeEach nodes as well this can be added. Removing it is harder if you wanna maintain backwards compatibility as much as possible.
4. I think this sounds reasonable.

Maybe it helps if I just write three decorators that I come up with now on the spot.

case 1: a global/fixed context value that transports a user's identity (lets say, a JWT). Tests use one of several fixed JWTs. I would write it like this:

func AsAdmin(ctx context.Context) context.Context {
	return context.WithValue(ctx, "identity", "admin1")
}

func AsMember(ctx context.Context) context.Context {
	return context.WithValue(ctx, "identity", "member1")
}

var _ = Describe("admin panel", func() {
	var hdl http.Handler
	BeforeEach(func() {
		hdl = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			fmt.Fprintf(w, "Hello, %s", r.Context().Value("identity"))
		})
	})
        
	Describe("as admin", AsAdmin, func() {
		It("should be able to access admin panel", func(ctx context.Context) {
			rec, req := httptest.NewRecorder(), httptest.NewRequest("GET", "/admin", nil)
			req = req.WithContext(ctx)
			hdl.ServeHTTP(rec, req)

			Expect(rec.Body.String()).To(Equal("Hello, admin1"))
		})

		It("should be able to edit settings", func(ctx context.Context) {
			rec, req := httptest.NewRecorder(), httptest.NewRequest("GET", "/admin/settings", nil)
			req = req.WithContext(ctx)
			hdl.ServeHTTP(rec, req)

			Expect(rec.Body.String()).To(Equal("Hello, admin1"))
		})
	})

	Describe("as user", AsMember, func() {
		It("should not be able to access admin panel", func(ctx context.Context) {
			rec, req := httptest.NewRecorder(), httptest.NewRequest("GET", "/admin", nil)
			req = req.WithContext(ctx)
			hdl.ServeHTTP(rec, req)

			Expect(rec.Body.String()).To(Equal("Hello, member1"))
		})

		It("should not be able to edit settings", func(ctx context.Context) {
			rec, req := httptest.NewRecorder(), httptest.NewRequest("GET", "/admin", nil)
			req = req.WithContext(ctx)
			hdl.ServeHTTP(rec, req)

			Expect(rec.Body.String()).To(Equal("Hello, member1"))
		})
	})
})

case 2: a dynamic setup. The context needs to transport something that is setup in a beforeEach. for example, a logger or a transaction. I would expect the following to work.

func WithK1Field(logs *zap.Logger) func(ctx context.Context) context.Context {
	return func(ctx context.Context) context.Context {
		return context.WithValue(ctx, "logs", logs)
	}
}

var _ = Describe("logging", func() {
	var logs *zap.Logger
	BeforeEach(func() {
		// instead of lo.Must is there a way to assert the tuple (zap.Logger, error) and fail when there is
		// an error but still return the value to be set? Could be a nice case for type constraints maybe?
		logs = lo.Must(zap.NewDevelopment())
		logs.With(zap.String("k1", "v1"))
	})

	// I think maybe this is where your points become tricky. Not sure if this proposal would allow
	// for capturing "logs" like this. Since it is set only in the BeforeEach. Passing it by reference
	// might still work?
	Describe("admin", WithK1Field(logs), func() {
		It("should have a logger", func(ctx context.Context) {
			logs := ctx.Value("logs").(*zap.Logger)
			Expect(logs).NotTo(BeNil())

			logs.Info("hello") // should include the k1=v1 field
		})
	})
})

case 3: a highly nested setup that has various layers of such decorators.

func Fielded(logs *zap.Logger, k, v string) func(ctx context.Context) context.Context {
	return func(ctx context.Context) context.Context {
		return context.WithValue(ctx, "logs", logs.With(zap.String(k, v)))
	}
}

var _ = Describe("nested", func() {
	var logs *zap.Logger
	BeforeEach(func() {
		logs = lo.Must(zap.NewDevelopment())
	})

	Describe("1", Fielded(logs, "k1", "v1"), func() {
		Describe("2", Fielded(logs, "k2", "v2"), func() {
			Describe("3", Fielded(logs, "k3", "v3"), func(ctx context.Context) {
				logs := ctx.Value("logs").(*zap.Logger)
				logs.Info("what now?") // would it have k1=v1,k2=v2,k3=v3?
			})
		})
	})
})

[onsi]

hey thanks for these examples they are super helpful.

case 1 is straightforward and makes sense. though things like "log in as admin" vs "log in as user" really belong in a BeforeEach (to my eye - subjective etc.)

case 2 starts to get at some of the complexity and this is the thing i'm worried about as i see people running into this all the time with how DescribeTable interacts with BeforeEach the logs passed in to WithK1Field(logs) has not been initialized and will have the value nil.

case 3 has a similar issue and it really feels (again, subjective) like these things should be in a BeforeEach. also is there a subtle issue here where the spec would only see k3=v3? I'm assuming logs.With() returns a new logs object with k=v but doesn't mutate logs itself? in which case since context.WithValue overwrites the "logs" key you'd only get the last logs.With(...) which would be k3=v3

I'm wondering what a better tool/pattern might be but i'm not sure yet. i'll keep thinking on it...