-
Notifications
You must be signed in to change notification settings - Fork 14.6k
MINOR: Add Unit test for TimingWheel
#20443
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
MINOR: Add Unit test for TimingWheel
#20443
Conversation
TimingWheel
TimingWheel
TimingWheel
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.
@m1a2st thanks for this patch. it looks great!
private final long tickMs = 10L; | ||
private final int wheelSize = 5; | ||
|
||
@BeforeEach |
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.
those variables could be initialized in constructor, right?
private final long startMs = 1000L;
private final long tickMs = 10L;
private final int wheelSize = 5;
private final DelayQueue<TimerTaskList> queue = new DelayQueue<>();
private final AtomicInteger taskCounter = new AtomicInteger(0);
private final TimingWheel timingWheel = new TimingWheel(tickMs, wheelSize, startMs, taskCounter, queue);
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.
In the separate test we will use these variables. If we initialize them in the constructor, we would need to add a test-only method for each variable, correct?
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.
The default Junit 5 lifecycle is PER_METHOD
, meaning each test case has its own test instance. That is why I suggest initializing those variables in the consutrcotr
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
|
There is any unit test for
TimingWheel
, we should add test for it.Reviewers: Chia-Ping Tsai [email protected]