test(core): split large inline test modules - #107
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
makoMakoGo
left a comment
There was a problem hiding this comment.
Reviewed against #95.
No blocking findings.
What I checked:
- #95 asks for a pure test-file organization change: move the large inline test modules out of
crates/tokscale-core/src/lib.rsandcrates/tokscale-core/src/pricing/lookup.rs, without changing implementation logic or opportunistically refactoring tests. - This PR only changes the expected four files:
lib.rs,lib_tests.rs,pricing/lookup.rs, andpricing/lookup_tests.rs. - The implementation files now only retain the
#[cfg(test)] #[path = ...] mod tests;hook, while the moved test files preserve the same parent-module access pattern (use super::.../use super::*). This is the cleanest version of the split within the scope of the issue; no extra abstraction or compatibility layer was introduced. - The compare stats are consistent with a pure extraction rather than behavior work: large deletions from the two implementation files, matching additions in the two new test files, and no unrelated files touched.
- The PR-reported validation says the
tokscale-coretest listing stayed unchanged, and the current head has a successful GitHub Actions test workflow.
Because the PR is still draft and stacked on #106, I am leaving this as a non-blocking review comment rather than an approval. Once #106 lands or this PR is retargeted to the landing branch, I would be comfortable approving this.
22b3221 to
212d84b
Compare
b1f0367 to
d585eef
Compare
makoMakoGo
left a comment
There was a problem hiding this comment.
Re-reviewed after the rebuild onto the current personal/local-clients baseline.
GitHub will not let the current account approve this PR because it is treated as the PR owner's account, but from code-review perspective this is approved.
Reasoning:
- The PR now targets
personal/local-clientsdirectly and is one commit ahead of that base. - The diff is still exactly the narrow #95 shape: only
crates/tokscale-core/src/lib.rs,crates/tokscale-core/src/lib_tests.rs,crates/tokscale-core/src/pricing/lookup.rs, andcrates/tokscale-core/src/pricing/lookup_tests.rschanged. - The implementation files now retain only the
#[cfg(test)] #[path = ...] mod tests;hooks. That preserves the tests as child modules of the original implementation modules, so private access and test names stay stable without adding abstraction. - The extracted test files keep the same
use super::.../use super::*access model and appear to be a pure module-boundary move rather than a semantic rewrite. - This matches the issue decision: split oversized inline tests out of implementation files, do not change implementation logic, do not opportunistically refactor tests.
- Current head CI is green, and the PR validation reports the core test list count still matches the current baseline.
No blocking findings.
Summary
tokscale-coreroot test module intosrc/lib_tests.rs.src/pricing/lookup_tests.rs.Closes #95
Built on the current
personal/local-clientsbaseline.Validation
cargo test -p tokscale-core -- --list | wc -l-> 1084, matching the currentpersonal/local-clientsbaselinecargo fmt --all --checkcargo clippy --workspace --all-targetscargo test --workspace(1886 passed, 4 ignored)wc -l crates/tokscale-core/src/lib.rs crates/tokscale-core/src/lib_tests.rs crates/tokscale-core/src/pricing/lookup.rs crates/tokscale-core/src/pricing/lookup_tests.rs:crates/tokscale-core/src/lib.rs-> 1288crates/tokscale-core/src/lib_tests.rs-> 5604crates/tokscale-core/src/pricing/lookup.rs-> 1932crates/tokscale-core/src/pricing/lookup_tests.rs-> 3322git diff --name-status fork/personal/local-clients...HEADshows only:crates/tokscale-core/src/lib.rscrates/tokscale-core/src/lib_tests.rscrates/tokscale-core/src/pricing/lookup.rscrates/tokscale-core/src/pricing/lookup_tests.rs