-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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 tests for uber applier #16154
add tests for uber applier #16154
Conversation
}, | ||
}}}, | ||
expectError: nil, | ||
}, |
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.
Could you please add an conditional delete in the Txn test case? It was the motivation of PR #13435
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.
Sure. Lemme check the background first.
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.
I can add a test case, but tbh I don't think that's the part of function I am trying to cover here......a better place would be the unit test of the server/storage package. However that package has no test coverage either. So nevermind, I will add it.
Signed-off-by: caojiamingalan <alan.c.19971111@gmail.com>
72b4dfb
to
ffe73f9
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, thanks @CaojiamingAlan !
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 - Thanks @CaojiamingAlan.
#16036
Add tests for uber applier.
Test cases involve building correct applier chains when alarms are on and off.