test: Improve Test Coverage of model package #367
Conversation
| // mockgen -destination mocks/observer.go -package mocks . Observer | ||
| // | ||
|
|
||
| // Package mock_observer is a generated GoMock package. | ||
| package mock_observer | ||
| // Package mocks is a generated GoMock package. | ||
| package mocks |
There was a problem hiding this comment.
Paired with @mike-zorn on batching all the mock objects in models together in one directory and re-ran codegen.
| @@ -1,8 +1,9 @@ | |||
| package model | |||
| package model_test | |||
There was a problem hiding this comment.
Wanted all the tests to be in the same package.
| project, err := store.GetDevProject(ctx, projectKey) | ||
| if err != nil { | ||
| return Override{}, errors.Wrap(err, "unable to get project") | ||
| } | ||
|
|
||
| observers := GetObserversFromContext(ctx) |
There was a problem hiding this comment.
Moved this call closer to where it's actually used.
| @@ -36,6 +38,7 @@ func CreateProject(ctx context.Context, projectKey, sourceEnvironmentKey string, | |||
| project.FlagState = flagsState | |||
| project.LastSyncTime = time.Now() | |||
|
|
|||
| store := StoreFromContext(ctx) | |||
There was a problem hiding this comment.
Just a cleanup - bundled the project variable initialization together into one statement and also moved the store fetch closer to where the store is used.
| @@ -71,7 +74,7 @@ func UpdateProject(ctx context.Context, projectKey string, context *ldcontext.Co | |||
| return Project{}, err | |||
| } | |||
| if !updated { | |||
| return Project{}, err | |||
| return Project{}, errors.New("Project not updated") | |||
There was a problem hiding this comment.
For UpdateProject, not returning an error here if the project wasn't actually updated seems like a bug? I updated this and SyncProject to return an error in this case, as well as coverage for that case. However, I definitely lack a lot of context so lemme know if there's something I'm missing.
There was a problem hiding this comment.
Oh, yeah. that makes sense. TBH we should probably change the store interface so that we return an error instead of the boolean. Issue is that if no rows are updated when we try and update, it means that we couldn't find the project.
tvarney13
changed the title
| expectedVal = ldvalue.Float64(99.99) | ||
| case "jsonFlag": | ||
| expectedVal = ldvalue.CopyArbitraryValue(map[string]any{"cat": "hat"}) | ||
| } |
There was a problem hiding this comment.
| } | |
| default: | |
| assert.Failf(t, "unknown flag, %s", key) | |
| } |
tvarney13
deleted the
tomvarney/sc-248911/improve-test-coverage-of-model-package
branch
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Title explains it. I also made some minor modifications I'll call out in PR comments.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.