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.

Sign up for GitHub

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

Jump to bottom

Node builder pattern for running test nodes in tests #1883

Merged
merged 16 commits into from
Oct 13, 2023
Merged

Node builder pattern for running test nodes in tests #1883

merged 16 commits into from
Oct 13, 2023

Conversation

ganeshvanahalli
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli commented Sep 26, 2023 •
edited
Loading

Current builder pattern is defined inside common_test.go

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Sep 26, 2023
tsahee
tsahee reviewed Sep 27, 2023
View reviewed changes
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

I like the general direction, but I do have many further suggestions. Would love to hear more views.

system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/batch_poster_test.go Outdated Show resolved Hide resolved
system_tests/batch_poster_test.go Outdated Show resolved Hide resolved
system_tests/batch_poster_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go Outdated Show resolved Hide resolved
@ganeshvanahalli ganeshvanahalli marked this pull request as ready for review September 29, 2023 22:17
tsahee
tsahee reviewed Sep 29, 2023
View reviewed changes
system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/bloom_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/batch_poster_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go Outdated Show resolved Hide resolved
@Tristan-Wilson Tristan-Wilson self-requested a review October 2, 2023 18:07
@ganeshvanahalli ganeshvanahalli requested a review from tsahee October 3, 2023 15:01
tsahee
tsahee reviewed Oct 3, 2023
View reviewed changes
system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go Outdated Show resolved Hide resolved
@ganeshvanahalli ganeshvanahalli requested a review from tsahee October 4, 2023 20:53
tsahee
tsahee reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

I really like it!
tiny/future comments.
Time to collect input from other programmers :)

system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go Outdated Show resolved Hide resolved
system_tests/common_test.go
dataDir string
isSequencer bool
takeOwnership bool
withL1 bool
Copy link
Contributor

Choose a reason for hiding this comment

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

note: in a later PR - we'll also want to incorporate ValidationNode here.. not sure if it'll be in a TestClient or Builder..
What we currently have is a weird/bad workaround.

joshuacolvin0
joshuacolvin0 approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@joshuacolvin0 joshuacolvin0 left a comment

Choose a reason for hiding this comment

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

LGTM

@joshuacolvin0 joshuacolvin0 merged commit ac85c0a into master Oct 13, 2023
7 checks passed
@joshuacolvin0 joshuacolvin0 deleted the test-node-builder branch October 13, 2023 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsahee tsahee tsahee left review comments

@joshuacolvin0 joshuacolvin0 joshuacolvin0 approved these changes

@PlasmaPower PlasmaPower Awaiting requested review from PlasmaPower

@Tristan-Wilson Tristan-Wilson Awaiting requested review from Tristan-Wilson

Assignees
No one assigned
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ganeshvanahalli @joshuacolvin0 @tsahee