DRY up config in tests. #2162
DRY up config in tests. #2162
Conversation
| DBSecretKey: defaultConfig.DBSecretKey, | ||
| UserAgentComment: defaultConfig.UserAgentComment, | ||
| Crontab: defaultConfig.Crontab, | ||
| }, |
There was a problem hiding this comment.
What I'd meant was to "DRY"-up these repeated Config structs given we're only changing two fields.
There was a problem hiding this comment.
Ooook, now I see your point; totally agree with you. Since we already have the default config, there's no need to repeat it in each test. Thanks!
| @@ -979,12 +842,10 @@ func Test_newSyncJob(t *testing.T) { | |||
|
|
|||
| for _, tt := range tests { | |||
| t.Run(tt.name, func(t *testing.T) { | |||
| if tt.config.UserAgentComment != "" { | |||
| defaultConfig.UserAgentComment = tt.config.UserAgentComment | |||
| defer func() { defaultConfig.UserAgentComment = "" }() | |||
There was a problem hiding this comment.
It wasn't clear to me why you were modifying defaultConfig at all here, given that you pass tt.config into your function below. But either way, although it's an interesting use of defer to reset the shared state for your test functions, it's a much better idea (IMO) to not have any shared state that requires resetting if you can avoid it (eg. what I did here by having each test construct the Config for each test).
There was a problem hiding this comment.
I didn't know either what was the reason for having this defer here. I just supposed it was the proper way to reset the state and I didn't want to change more things in the previous PR.
However, with this change, it is much clearer now :)
| defaultConfig.Crontab = tt.config.Crontab | ||
| defer func() { defaultConfig.Crontab = "" }() | ||
| } | ||
| result := newCronJob(tt.apprepo, tt.config) |
There was a problem hiding this comment.
See comment below about the defer funcs here.
Description of the change
Just a follow-up of #2156 (comment) (sorry, wasn't very clear with what I meant there), removing the extra code which is repeated in many tests.
Also removes the defers which were used to reset test config, instead creating the test config (functionally) with no state shared between tests.