Conversation
kiootic
left a comment
There was a problem hiding this comment.
I'm not seeing where the job status is updated?
Also, we can refactor the synchronizer to make it easier to test:
- Extract the webhook server, so that the Synchronizer would accept the two channels instead.
- Extract a
Clockinterface to make time-based tests faster.
pkg/github/jobs/synchronizer_test.go
Outdated
| "gopkg.in/h2non/gock.v1" | ||
| ) | ||
|
|
||
| func Ptr[T any](v T) *T { |
| } | ||
| func TestRun(t *testing.T) { | ||
|
|
||
| logger, _ := zap.NewProduction() |
There was a problem hiding this comment.
Can use NewNop to avoid polluting output.
| @@ -0,0 +1,137 @@ | |||
| package jobs | |||
There was a problem hiding this comment.
Use jobs_test to avoid using package internal functions.
There was a problem hiding this comment.
if I alter it, I can't create a webhookObject for testing. Should I create another public function sth like NewWebhookObject()
pkg/github/jobs/synchronizer_test.go
Outdated
| runs := make(chan webhookObject[*github.WorkflowRun]) | ||
| jobs := make(chan webhookObject[*github.WorkflowJob]) | ||
| s, _ := NewSynchronizer(logger, config, client, kv, registry) | ||
| go s.run(ctx, runs, jobs) |
There was a problem hiding this comment.
We should be testing the public interface of the unit, so should use Start instead.
Can you explain more about the |
You can refer here for more details: https://stackoverflow.com/a/18970352 Basically, it would be hard to test time related logic, since 'time' is a global state. To make it easier to test, we uses global time functions through an abstraction, so that we can replace it during testing. |
|
To alter the package to |
reference site: https://github.com/h2non/gock