Split lib test suites into integration targets (Fixes #307)#368
Split lib test suites into integration targets (Fixes #307)#368Ayush7614 wants to merge 3 commits into
Conversation
Move GitHub client, layout, and other public behavioral suites out of the lib harness so Clippy's large_stack_arrays ceiling no longer blocks new independent tests. Document the per-target registration budget.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pub mod logging; | ||
| /// Single-pass HTML-to-text stripping for untrusted markdown (issue #155). | ||
| pub(crate) mod markdown_html_strip; | ||
| pub mod markdown_html_strip; |
There was a problem hiding this comment.
Expanding
markdown_html_stripfrompub(crate)topubadds new public API surface (strip_html_to_text,contains_open_tag). Because this crate is published (jefe0.0.29), this is a semver-breaking API addition. If the intent is to expose it, consider adding explicit public docs/stability notes and a feature gate; otherwise keep itpub(crate).
| fn rest_page_1_with_more_yields_page_2() { | ||
| let token = PageToken::after_page(1, true); | ||
| assert_eq!(token, PageToken::PageNumber(2)); | ||
| } |
There was a problem hiding this comment.
Missing boundary test for
PageToken::after_pageoverflow:after_page(u32::MAX, true)should yieldDonebecausepage.checked_add(1)wraps toNone. Without this test, an off-by-one or overflow regression in theafter_pageimplementation would go undetected.
| #[test] | ||
| fn list_request_id_default_is_zero() { | ||
| assert_eq!(ListRequestId::default().get(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn list_request_id_checked_next_increments() { | ||
| let id = ListRequestId::from_raw(41); | ||
| assert_eq!(id.checked_next(), Some(ListRequestId::from_raw(42))); | ||
| } |
There was a problem hiding this comment.
Missing test for
ListRequestId::from_raw(0).checked_next()and explicit consistency betweenDefaultandfrom_raw(0). The current tests verifyfrom_raw(41)andfrom_raw(u64::MAX), but skip the zero boundary. Adding a test thatdefault() == from_raw(0)and thatfrom_raw(0).checked_next()yieldsSome(from_raw(1))would close this gap.
OpenCodeReview — PR #368
|
|
@Ayush7614 failing ci |
…ck#307) Resolve conflicts with the landed rewrite/stateReason work, keep the GitHub field-list regression in the integration target, document the markdown_html_strip pub surface, and add pagination boundary coverage.
Keep the lib harness under Clippy's large_stack_arrays ceiling after merging main, which reintroduced enough registrations to exceed 2048.
| fn parse(args: &[&str]) -> Result<CliArgs, CliError> { | ||
| parse_args(args.iter().map(|s| (*s).to_string())) | ||
| } |
There was a problem hiding this comment.
The
parsetest helper allocates aStringfor every argument via.to_string(), butparse_argsalready acceptsS: Into<String>and&strimplements that trait directly. This extra allocation is unnecessary in test code. You can avoid it by usingargs.iter().copied()to pass&strslices directly, lettingInto::intoperform the conversion when needed.
| assert_eq!(parsed, CliArgs::default()); | ||
| assert!(!parsed.version); | ||
| assert!(!parsed.help); | ||
| assert!(parsed.config_dir.is_none()); |
There was a problem hiding this comment.
The assertions
!parsed.version,!parsed.help, andparsed.config_dir.is_none()are redundant because the precedingassert_eq!(parsed, CliArgs::default())already verifies every field matches the default. If you want explicit field-level documentation, consider checking only the fields that are non-default or removing the redundant checks entirely.
| #[test] | ||
| fn entity_window_on_multibyte_boundary_does_not_panic() { | ||
| let input = format!("&{}中", "a".repeat(MAX_ENTITY_LEN - 2)); | ||
| let input = format!("&{}中", "a".repeat(12 - 2)); |
There was a problem hiding this comment.
The hardcoded literal
12replaces the previously referencedMAX_ENTITY_LENconstant. This creates a silent maintenance hazard: ifMAX_ENTITY_LENis ever changed, this test will no longer verify the multibyte boundary condition it was designed to check, because the input length will no longer align with the actual entity-scan window. Import and use the constant instead, e.g.:use jefe::markdown_html_strip::{contains_open_tag, strip_html_to_text, MAX_ENTITY_LEN};
| assert_eq!(out, input, "unterminated entity passes through: {out:?}"); | ||
| // Same shape with a 4-byte emoji on the boundary. | ||
| let input2 = format!("&{}😀", "a".repeat(MAX_ENTITY_LEN - 3)); | ||
| let input2 = format!("&{}😀", "a".repeat(12 - 3)); |
There was a problem hiding this comment.
Same issue: hardcoded
12 - 3should referenceMAX_ENTITY_LENso the test adapts if the entity window size changes.
|
@acoliver — CI update for this PR: Green: fmt, clippy, source-length, coverage, Linux Build + Test, OCR. Mergeable with no conflicts. Lib registrations are under the Clippy descriptor ceiling (1996 / 2048 on Windows lib run). Only red check: Native Windows (MSVC + psmux). Root cause (not from the test-target split):
I don’t have rights to re-run failed jobs on upstream Actions. Could you please re-run the Native Windows job (or merge if you’re OK treating that flake as known)? Happy to iterate if you want anything else changed here. |
|
@Ayush7614 I'll fix the win stuff probably in the am and merge this after. Thanks! |
2 participants
Summary
tests/*.rstargets (same pattern asgit_info).large_stack_arrayson the generated test descriptor array.dev-docs/standards/testing-and-quality.md.Test plan
cargo test --lib -- --list→ 1995cargo test --test github_client(+ other new targets)CLIPPY_CONF_DIR=.github/clippy cargo clippy --lib --tests --all-features -- -D warnings(nolarge_stack_arrays)make ci-checkFixes #307