BED-5478: Steel Thread Demo for Removing E2E tests #1202
Conversation
|
|
||
| otherUserID := "00000000-1111-2222-3333-444444444444" | ||
|
|
||
| tokenRequest := v2.CreateUserToken{ |
There was a problem hiding this comment.
In all instances where we're creating a payload from a defined package model, we should instead be creating new objects decoupled from the actual implementation.
The trouble with using the defined models for contract tests is that the model can be changed without triggering any difference in test behaviors - meaning the input contract can change without the test discovering it.
In place of creating these model instances, I'd recommend instead defining
tokenRequest := map[string]any{
"user_id": otherUserID,
"token_name": "test token",
}
json.Marshal will still marshal it into a format that can be passed forward into the request as it exists now
There was a problem hiding this comment.
I wouldn't worry about mock setups, but request payloads should follow this pattern. You'll find it in POST, PUT and PATCH requests
| mockDB.EXPECT().CreateAuthToken(gomock.Any(), gomock.Any()).Return(model.AuthToken{}, errors.New("failed to store token")) | ||
|
|
||
| if b, err := json.Marshal(tokenRequest); err != nil { | ||
| tc.Fatal(err) |
There was a problem hiding this comment.
Looking to switch if [...]; err != nil { pattern to
b, err := json.Marshal(tokenRequest)
require.NoError(err)
There was a problem hiding this comment.
Just easier to read methinks, and since we're providing the example others will follow we'll want to be consistent in the patterns we use
| @@ -3218,11 +3219,161 @@ func TestManagementResource_CreateAuthToken(t *testing.T) { | |||
| } | |||
| }) | |||
|
|
|||
| t.Run("User not allowed to create token", func(tc *testing.T) {}) | |||
| t.Run("User not allowed to create token", func(tc *testing.T) { | |||
There was a problem hiding this comment.
While we're using two different patterns for multi-testing around t.Run, I feel this is a perfectly valid use to test thru CreateAuthToken
We'll want to make comments in the recommended practices doc as to why to choose one pattern over the other, or when to use multi-testing at all.
In my mind the value of this use here is that the intent is perfectly well communicated - different test cases on a single endpoint, where setup is small and repeated code presents no pain.
How I'm using it in saved_queries_test.go I feel is more aptly suited to cases where there's more benefit to be found in consolidating code (tho right now it's not doing much of anything over there except presenting the pattern ...) - the intent is slightly less clear, but in cases where test setup is significant it should be easier to maintain when tests need to change.
I'm happy to showcase both patterns
There was a problem hiding this comment.
I still need to clean up my side of how matrix testing is used.
I think it does a decent enough job of showing how to run a matrix test, but doesn't at all communicate the why of it
| require.Equal(tc, expectedToken.Name.String, responseToken.Name.String) | ||
| } else { | ||
| require.Equal(tc, expectedToken.Name.String, responseWrapper.Data.Name.String) | ||
| } |
There was a problem hiding this comment.
I'd avoid flow control in our tests. If the test is non-deterministic like this we should have two tests instead of trying to manage two response states in one.
| rec := httptest.NewRecorder() | ||
| localRouter.ServeHTTP(rec, req) | ||
|
|
||
| require.Equal(tc, http.StatusInternalServerError, rec.Code) |
There was a problem hiding this comment.
I would take a look at how we're using assert.JSONEq in saved_queries_test.go, it's pretty slick and lets us define more complete response checks against our API.
Description
Steel thread demo with @sircodemane for
CreateAuthTokenendpoint to demonstrate what a contract unit test looks likeMotivation and Context
This PR addresses: BED-5478
How Has This Been Tested?
All tests passed
Screenshots (optional):
Types of changes
Checklist: