Skip to content
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

Add jobs synchronizer test #10

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add jobs synchronizer test #10

wants to merge 6 commits into from

Conversation

Ngkaokis
Copy link
Contributor

Copy link
Collaborator

@kiootic kiootic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Clock interface to make time-based tests faster.

"gopkg.in/h2non/gock.v1"
)

func Ptr[T any](v T) *T {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be private function.

}
func TestRun(t *testing.T) {

logger, _ := zap.NewProduction()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use NewNop to avoid polluting output.

@@ -0,0 +1,137 @@
package jobs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use jobs_test to avoid using package internal functions.

Copy link
Contributor Author

@Ngkaokis Ngkaokis Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I alter it, I can't create a webhookObject for testing. Should I create another public function sth like NewWebhookObject()

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be testing the public interface of the unit, so should use Start instead.

@Ngkaokis
Copy link
Contributor Author

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 `Clock` interface to make time-based tests faster.

Can you explain more about the Clock interface?

@kiootic
Copy link
Collaborator

kiootic commented Jul 12, 2022

Can you explain more about the Clock interface?

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.

@Ngkaokis
Copy link
Contributor Author

To alter the package to jobs_test, how to handle the situation that package internal functions/properties are needed (e.g. create webhook object, assert the result)? Should I create getter for those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants