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

Tests for builder service #11214

Merged
merged 17 commits into from
Sep 6, 2022
Merged

Tests for builder service #11214

merged 17 commits into from
Sep 6, 2022

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Aug 12, 2022

What type of PR is this?

Testing

What does this PR do? Why is it needed?

Adds tests to non-trivial functions of beacon-chain/builder/service.go. To make the service testable, a new BuilderClient interface was created and the tight coupling between a concrete implementation of the builder and the service was removed. Currently a new builder is instantiated directly in the service's constructor. This PR shifts dependencies and makes the builder configurable via an option.

@rkapka rkapka requested a review from a team as a code owner August 12, 2022 14:50
@rkapka rkapka added Ready For Review A pull request ready for code review Builder PR or issue that supports builder related work labels Aug 12, 2022
terencechain
terencechain previously approved these changes Aug 12, 2022
@@ -295,3 +307,12 @@ func non200Err(response *http.Response) error {
return errors.Wrap(ErrNotOK, errMessage.Message)
}
}

func covertEndPoint(ep string) network.Endpoint {
Copy link
Member

Choose a reason for hiding this comment

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

This would be useful as a general helper in a common location, we do something like this in execution service as well. Another PR will be great for this

terencechain
terencechain previously approved these changes Aug 12, 2022
@rkapka rkapka removed the Ready For Review A pull request ready for code review label Aug 12, 2022
@rkapka
Copy link
Contributor Author

rkapka commented Aug 12, 2022

Removing Ready for Review because I broke e2e

rkapka added 2 commits August 16, 2022 15:38
# Conflicts:
#	api/client/builder/client.go
#	beacon-chain/builder/option.go
#	beacon-chain/builder/service.go
@rauljordan
Copy link
Contributor

Tests fail @rkapka

@rkapka rkapka added the Ready For Review A pull request ready for code review label Sep 6, 2022
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Thank you!

@prylabs-bulldozer prylabs-bulldozer bot merged commit b4d2395 into develop Sep 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the builder-service-tests branch September 6, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builder PR or issue that supports builder related work Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants