-
-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement generic transformer decorator: ginkgo.ContextTransformer #1404
Comments
hey I'm thinking about implementing this next. if a node is passed a new
then any child nodes that accept a There are a few caveats that might confuse some users so would appreciate thoughts on these:
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:
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. 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 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 case 3 has a similar issue and it really feels (again, subjective) like these things should be in a I'm wondering what a better tool/pattern might be but i'm not sure yet. i'll keep thinking on it... |
I came looking to see if there was an already implemented solution and found this thread. I think case 1 makes the most sense. Here is a simplified version of test-code my team is writing today: usersettings.gotype identityKey string
const ctxIdentityKey identityKey = "identity-key"
func UserSettingsHandler() http.Handler {
mux := http.NewServeMux()
mux.HandleFunc("/user/{id}/settings", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
log := zerolog.Ctx(ctx)
ctxIdentity, ok := ctx.Value(ctxIdentityKey).(string)
requestedIdentity := r.PathValue("id")
if !ok || requestedIdentity != ctxIdentity {
log.Error().Msg("invalid situation")
w.WriteHeader(http.StatusUnauthorized)
w.Write([]byte("no can do"))
return
}
log.Error().Msg("valid situation")
w.WriteHeader(http.StatusAccepted)
w.Write([]byte(fmt.Sprintf("no problem %s", ctxIdentity)))
}))
return mux
} usersettings_test.govar _ = Describe("With a user", func() {
var testCtx context.Context
BeforeEach(func() {
testCtx = context.Background()
})
Describe("isn't logged in", func() {
It("should not allow updates", func() {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(testCtx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no can do"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusUnauthorized))
})
})
Describe("tries to change another user's settings", func() {
BeforeEach(func() {
testCtx = context.WithValue(testCtx, ctxIdentityKey, "5678")
})
It("should not allow updates", func() {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(testCtx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no can do"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusUnauthorized))
})
})
Describe("who is logged in", func() {
BeforeEach(func() {
testCtx = context.WithValue(testCtx, ctxIdentityKey, "1337")
})
It("should update settings", func() {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(testCtx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
})
})
}) It does get very cumbersome when setting up multiples of things (zerologger, otel, etc) but more importantly becomes troublesome when we want to handle test cancelations cleanly. EG: It("should update settings", func(ctx SpecContext) {
// sans importing a third party library, you must handle these two
// contexts manually.
testCtx, cancelCtx := context.WithCancel(testCtx)
defer cancelCtx()
go func() {
defer cancelCtx()
<-ctx.Done()
}()
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(testCtx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
}) With case 1, it would be trivial to manage cases like this. func WithUser(id string) func(ctx context.Context) context.Context {
return func(ctx context.Context) context.Context {
return context.WithValue(ctx, ctxIdentityKey, id)
}
}
var _ = Describe("With a user", func() {
Describe("isn't logged in", func() {
It("should not allow updates", func(ctx SpecContext) {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(ctx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no can do"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusUnauthorized))
})
})
Describe("tries to change another user's settings", func() {
BeforeEach(WithUser("5678"), func(){
// potentially do more setup here
})
It("should not allow updates", func(ctx SpecContext) {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(ctx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no can do"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusUnauthorized))
})
})
Describe("who is logged in", func() {
BeforeEach(WithUser("1337"), func() {
// potentially do more setup here
})
It("should update settings", func(ctx SpecContext /*this context is ginkgo cancelable*/) {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(ctx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
})
})
}) In terms of order-of-operations, I imagine that this would operate in the same precedence as their nodes. So something like this could be achieved func WithZerolog(configFilename string) func(ctx context.Context) context.Context {
return func(ctx context.Context) context.Context {
config, err := loadConfig(configFilename)
Expect(err).ShouldNot(HaveOccurred(), "no test config")
consoleWriter := zerolog.ConsoleWriter{Out: os.Stdout}
ctx = zerolog.New(consoleWriter).Level(config.Level).WithContext(ctx)
return ctx
}
}
var _ = Describe("With a user", func() {
BeforeEach(WithZerolog("my-test-config.yml"), func(ctx SpecContext) {
log := zerolog.Ctx(ctx)
log.Debug().Msg("starting tests")
})
// ...
Describe("who is logged in", func() {
It("should update settings", WithUser("5678"), func(ctx SpecContext /*this context is ginkgo cancelable*/) {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(ctx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
})
})
}) If n number of these transformers were allowed as a decorator, you could get the behavior described in 2 and I think 3 as well. Even if that's not desirable chaining higher order functions should still have the same effect, but it would likely be less error prone if this were done under the hood within the Ginkgo framework instead of by all library users. func asPointer[T any](v T) *T {
return &v
}
var _ = Describe("With a user", func() {
var targetUser *string
BeforeEach(
WithZerolog("my-test-config.yml"),
func(ctx context.Context) context.Context {
targetUser = asPointer(uuid.NewString())
ctx = zerolog.Ctx(ctx).With().Str("target-user", *targetUser).Logger().WithContext(ctx)
return ctx
}, func(ctx SpecContext) {
log := zerolog.Ctx(ctx)
log.Debug().Msg("starting tests")
},
)
// ...
Describe("who is logged in", func() {
BeforeEach(func(ctx context.Context) context.Context {
return context.WithValue(ctx, ctxIdentityKey, *targetUser)
})
It("should update settings", func(ctx SpecContext /*this context is ginkgo cancelable*/) {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", fmt.Sprintf("/user/%s/settings", *targetUser), nil).WithContext(ctx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal(fmt.Sprintf("no problem %s", *targetUser)))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
})
})
}) |
hey @niko-dunixi - so this is the confusing behavior I'm concerned about: func WithZerolog(configFilename string) func(ctx context.Context) context.Context {
return func(ctx context.Context) context.Context {
config, err := loadConfig(configFilename)
Expect(err).ShouldNot(HaveOccurred(), "no test config")
consoleWriter := zerolog.ConsoleWriter{Out: os.Stdout}
ctx = zerolog.New(consoleWriter).Level(config.Level).WithContext(ctx)
return ctx
}
}
var _ = Describe("With a user", func() {
BeforeEach(WithZerolog("my-test-config.yml"), func(ctx SpecContext) {
log := zerolog.Ctx(ctx)
log.Debug().Msg("starting tests")
})
// ...
Describe("who is logged in", func() {
It("should update settings", WithUser("5678"), func(ctx SpecContext /*this context is ginkgo cancelable*/) {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(ctx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
})
})
}) assumes that that a single context (let's call it But that is not the current behavior. Each node gets its own context. So the You could pull things up to the var _ = Describe("With a user", WithZerolog("my-test-config.yml"), func() { //now everyone gets the zero-log version
BeforeEach(func(ctx SpecContext) {
log := zerolog.Ctx(ctx) //works
log.Debug().Msg("starting tests")
})
// ...
Describe("who is logged in", WithUser("5678"), func() { // all the specs in here get the WithUser version
It("should update settings", func(ctx SpecContext /*this context is ginkgo cancelable*/) {
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(ctx) //works
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
})
})
}) this will work but there will be an awkward subtlety. the If I had a time machine I would have implemented context differently. I would have added a Perhaps there's a GInkgo 3 someday that fixes this (or perhaps I bite the bullet and introduce such a change gradually within 2.x). But I wonder if, for now, there is a simpler approach. Let's go back to the super-ugly cumbersome piece: It("should update settings", func(ctx SpecContext) {
// sans importing a third party library, you must handle these two
// contexts manually.
testCtx, cancelCtx := context.WithCancel(testCtx)
defer cancelCtx()
go func() {
defer cancelCtx()
<-ctx.Done()
}()
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(testCtx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
}) what if, instead, you could: var testCtx context.Context
BeforeEach(func() {
testCtx = WithZerolog(context.Background(), "foo.log")
})
Describe("with a logged in user", func() {
BeforeEach(func() {
zerolog.Ctx(testCtx).Debug().Msg("starting logged-in user test")
testCtx = context.WithValue(testCtx, "user", "1337")
DeferCleanup(func() {
zerolog.Ctx(testCtx).Debug().Msg("finished logged-in user test")
})
})
It("should update settings", func(ctx SpecContext) {
ctx = ctx.Wrap(testCtx) // magic
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/settings", nil).WithContext(ctx)
UserSettingsHandler().ServeHTTP(recorder, request)
// Assertions
Expect(recorder.Body.String()).To(Equal("no problem 1337"))
Expect(recorder.Result().StatusCode).To(Equal(http.StatusAccepted))
})
}) Basically, If that isn't enough, then I could imagine that To my eye this approach reads fairly clearly. You can explicitly see that I could imagine this being useful for things like this: Describe("with dueling users", func() {
var adminCtx, hackerCtx context.Context
BeforeEach(func() {
adminCtx = context.WithValue(testCtx, "user", "1337")
hackerCtx = context.WithValue(testCtx, "user", "1138")
})
It("should not allow hackers to message admins", func(ctx SpecContext) {
// as the hacker
recorder := httptest.NewRecorder()
request := httptest.NewRequest("POST", "/user/1337/message", nil).WithContext(ctx.Wrap(hackerCtx))
UserSettingsHandler().ServeHTTP(recorder, request)
Expect(recorder.Body.String()).To(Equal("nope"))
//as the admin
recorder = httptest.NewRecorder()
request = httptest.NewRequest("GET", "/user/1337/messages", nil).WithContext(ctx.Wrap(adminCtx))
UserSettingsHandler().ServeHTTP(recorder, request)
Expect(recorder.Body.String()).To(Equal("no new messages"))
})
}) thoughts? |
@onsi That makes a lot more sense with that clarification. Thanks for taking the time to lay that out. I think there might be a couple of sub-problems we're teasing out. One of them being context management, and the other being reusable test configurations and setups. RE: ContextsI found that Maybe func (sctx SpecContext) ChainCancel(ctx context.Context) context.Context {
ctx, cancelCtx := context.WithCancelCause(ctx)
context.AfterFunc(sctx, func() {
cancelCtx(context.Cause(sctx))
})
return ctx
} Not hard-set on any name in particular. I definitely wouldn't recommend reverse propagation of cancelation. It seems like it would be more problematic than not. RE: Reusable test-fixturesMaybe we want to create a more broad/general pattern for creating reusable test setups, and contexts just happen to be a common/incidental piece of that puzzle. Here is a repurposed example of setting up an NGINX testcontainer. Our real Ginkgo tests we use have some proprietary configuration with Postgres and Redis, but the pattern should demonstrate our usage. https://github.com/niko-dunixi/ginkgo-testcontainers-example/blob/55bfe3308f8f41dd6d7ac5e74e8a009eeb906680/niginx_test.go#L11-L36func WithMyTestContainer(text string, specs ...func(nginxGetter Supplier[*nginxContainer])) {
GinkgoHelper()
Context(text, func() {
var nginxContainerInstance *nginxContainer
BeforeEach(func(ctx SpecContext) {
container, err := startContainer(ctx)
Expect(err).ShouldNot(HaveOccurred(), "could not setup nginx testcontainer")
Expect(container).ToNot(BeNil(), "nginx container should not be nil if there is no err")
nginxContainerInstance = container
DeferCleanup(func(ctx SpecContext) {
err := container.Terminate(ctx)
Expect(err).ShouldNot(HaveOccurred(), "could not cleanup nginx testcontainer")
}, NodeTimeout(time.Second*30))
}, NodeTimeout(time.Minute*5))
for _, spec := range specs {
spec(func() *nginxContainer {
// This must be a higher order function, otherwise we cannot pass the initialized
// value from the `BeforeEach` to the `It` with during the Test Execution phase
return nginxContainerInstance
})
}
})
} Which can now be used and reused in tests. https://github.com/niko-dunixi/ginkgo-testcontainers-example/blob/55bfe3308f8f41dd6d7ac5e74e8a009eeb906680/niginx_test.go#L38-L54var _ = Describe("some scenario where I do things", func() {
WithMyTestContainer("and the other service is available", func(nginxGetter Supplier[*nginxContainer]) {
// Using a BeforeEach for variable assignment.
// This helps tests conform to other ginkgo examples
// in the wild and help with debugability in a given test.
var nginx *nginxContainer
BeforeEach(func() {
nginx = nginxGetter()
})
It("should succeed", func(ctx SpecContext) {
response, err := http.Get(nginx.URI)
Expect(err).ShouldNot(HaveOccurred(), "nginx isn't working")
Expect(response.StatusCode).To(Equal(http.StatusOK), "nginx isn 't working.")
})
})
}) Maybe there is a better way [to do] reusable test-fixtures? Some pattern that we could use generically but also for contexts? Circling BackMaybe it's not possible to do; but I'm starting to think that a decorator isn't the best place to put some of this configuration. Ultimately I would be ecstatic if there was a way I could extend Ginkgo into any arbitrary setup that was slightly more ergonomic and user-friendly than my own solution. Something like this to tie it all together? var _ = MyCustomDescribe("When I have an admin logged in", func(
testScenarioCtx context.Context, // My custom Context, NOT a Ginkgo SpecContext
db *gorm.DB, // gorm initialized with a mock database
mock sqlmock.Sqlmock // sqlmock to actually setup expectations,
) {
BeforeEach(func() {
mock.ExpectExec("select 1;")
})
It("should do foo", func(sctx SpecContext) {
ctx := sctx.ChainCancel(testScenarioCtx)
err := db.WithContext(ctx).Exec("select 1;").Error
Expect(err).ShouldNot(HaveOccurred())
})
}) |
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):
It would be nice if we could do something like this:
@onsi proposed the following idea:
The text was updated successfully, but these errors were encountered: