-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(pops): Refactor Challenger for Testability #5624
Conversation
|
✅ Deploy Preview for opstack-docs canceled.
|
8923c81
to
35d4aa7
Compare
Hey @refcell! This PR has merge conflicts. Please fix them before continuing review. |
35d4aa7
to
4819cee
Compare
4819cee
to
1fe64d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Worth noting that the CheckRequired
method in flags.go
isn't being called now. The config checks pick it up but the error message they give is slightly less useful because it doesn't include the specific CLI option name that's missing (ie you get missing l1 eth rpc url
instead of flag --l1-eth-rpc is required
).
I suggest hooking that back up in the next PR and adding tests for the CLI flags to cover it. You do get a bit of duplication between the config tests and flag tests but they're quite simple tests and cli vs config are their own separate concerns so worth testing individually. Plus the CLI tests cover the actual parsing which can provide some surprises at times.
This PR has been added to the merge queue, and will be merged soon. |
This PR is next in line to be merged, and will be merged as soon as checks pass. |
This PR is next in line to be merged, and will be merged as soon as checks pass. |
Description
Cleans up the op-challenger to follow the
op-program
's format so we can test cli args easily.NOTE:
challenger/utils.go
will be removed in #5622 :)