Open advdv opened 2 months ago
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:
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.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
.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?Given all those pieces - does this still work for your usecase?
Cool! these are some interesting thoughts. I'm not that deep into the codebase but:
ContextWrapper(func(ctx SpecContext) context.Context)
but this makes it less flexibleSooo, 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.
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?
})
})
})
})
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...
This feature has been discussed in the following issue: https://github.com/onsi/ginkgo/issues/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):
It would be nice if we could do something like this:
@onsi proposed the following idea: