RockteMQ-AI commented on PR #10409:
URL: https://github.com/apache/rocketmq/pull/10409#issuecomment-4691770122
## Review by github-manager-bot
### Summary
Harden `UtilAll.isItTimeToDo` to silently skip
null/blank/invalid/out-of-range hour tokens instead of throwing
`NumberFormatException`, with new unit tests and unrelated doc fixes.
### Findings
- **[Critical]**
`common/src/test/java/org/apache/rocketmq/common/UtilAllTest.java:156` — The
`@Test` annotation is corrupted: the diff shows
`@repos/apache_rocketmq/auth/src/test/java/...` instead of `@Test`. This means
`testIsItTimeToDo` will not be recognized as a test method and will never run.
The entire test coverage rationale for this PR breaks down.
- **[Warning]**
`common/src/main/java/org/apache/rocketmq/common/UtilAll.java:120` — The outer
`if (whiles.length > 0)` guard is now dead code: `String.split(";")` on a
non-blank string always returns an array of length ≥ 1. Not a bug, but
misleading.
- **[Warning]**
`common/src/test/java/org/apache/rocketmq/common/UtilAllTest.java:157` — The
positive test `assertTrue(UtilAll.isItTimeToDo(String.valueOf(currentHour)))`
is time-sensitive and will fail if the hour rolls over between
`Calendar.getInstance()` and `isItTimeToDo`'s own `Calendar.getInstance()` call
(extremely rare, but flaky by design). Consider passing a fixed mock or
accepting the tiny risk explicitly.
- **[Warning]**
`common/src/test/java/org/apache/rocketmq/common/UtilAllTest.java:158` —
`assertFalse(UtilAll.isItTimeToDo("99"))` is correct for the out-of-range case,
but there is no `assertFalse` for a valid hour that is *not* the current hour
(e.g., `(currentHour + 1) % 24`). The negative path for a well-formed but
non-matching hour is untested.
- **[Info]**
`common/src/main/java/org/apache/rocketmq/common/UtilAll.java:130` — The catch
block comment ("Ignore invalid hour tokens to avoid breaking scheduled tasks.")
violates the project's no-obvious-comment style, but is borderline acceptable
given the silently-swallowed exception.
- **[Info]** Doc renames (`Design_LoadBlancing.md` →
`Design_LoadBalancing.md`, `Design_Trancation.md` → `Design_Transaction.md`,
`Troubleshoopting.md` → `Troubleshooting.md`) fix long-standing typos; these
are welcome cleanups but are unrelated to the stated issue scope.
### Suggestions
1. **Fix the `@Test` annotation immediately** — the annotation on
`testIsItTimeToDo` is the file path of an unrelated file
(`@repos/apache_rocketmq/auth/...`). This is the blocking issue; the PR's
primary value (test coverage) is entirely negated without it.
2. Add a `assertFalse(UtilAll.isItTimeToDo(String.valueOf((currentHour + 1)
% 24)))` case to cover the valid-but-non-matching hour path.
3. Remove the dead `if (whiles.length > 0)` wrapper, or at minimum flatten
the indentation, to keep the method readable.
---
*Automated review by github-manager-bot*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
