Conversation
Jorgedanisc
There was a problem hiding this comment.
Pull request overview
This PR expands test coverage across the DUC toolchain, improves robustness of Rust-side DUC deserialization for integer fields, adds page indexing to pdf2svg output, and updates release workflow ordering. It also introduces WebP MIME recognition in ducpdf’s resource streaming and bumps hipdf.
Changes:
- Update ducsv/ducpdf tests to run against all
.ducassets and write outputs intotests_output/. - Add
pageIndexto pdf2svg’s per-page JSON output and align Rust/TS serialization shape. - Add tolerant integer deserializers in
ducrs(truncate floats intoi32) and apply them to many schema fields; extend Rust test suite to validate assets and invariants.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ducsvg/tests/helpers.ts | Adds helper to enumerate .duc fixtures. |
| packages/ducsvg/tests/duc-to-svg.unit.test.ts | Runs crop/plot SVG conversion over all fixtures; writes outputs under tests_output/. |
| packages/ducsvg/tests/.gitignore | Ignores tests_output instead of output. |
| packages/ducsvg/src/pdf2svg/src/lib.rs | Emits pageIndex in serialized page objects. |
| packages/ducsvg/src/pdf2svg/index.ts | Updates TS types to include pageIndex. |
| packages/ducrs/src/types.rs | Applies truncating deserializers to numerous i32 fields. |
| packages/ducrs/src/serde_utils.rs | Introduces trunc_i32 / trunc_vec_i32 serde helpers. |
| packages/ducrs/src/lib.rs | Adds a broad Rust test suite covering parsing, round-trips, invariants, and DB bootstrap. |
| packages/ducpdf/tests/plots.test.ts | Runs plot-mode conversions over all .duc fixtures. |
| packages/ducpdf/tests/helpers.ts | Adds helper to enumerate .duc fixtures. |
| packages/ducpdf/src/duc2pdf/tests/test_integration.rs | Adds a “plot all assets” integration test using Rust converter. |
| packages/ducpdf/src/duc2pdf/src/streaming/stream_resources.rs | Adds WebP resource type detection and routing. |
| packages/ducpdf/src/duc2pdf/src/builder.rs | Adds image/webp to external-file image handling. |
| packages/ducpdf/src/duc2pdf/Cargo.toml | Bumps hipdf dependency version. |
| Cargo.toml | Removes an extra blank line. |
| Cargo.lock | Updates lockfile for hipdf bump and transitive deps. |
| .github/workflows/release.yml | Adjusts release job ordering/conditions to follow dependency order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
|
||
| match resource_type { | ||
| ResourceType::Svg => self.process_svg_file(file), | ||
| ResourceType::Png | ResourceType::Jpeg => { | ||
| ResourceType::Png | ResourceType::Jpeg | ResourceType::WebP => { | ||
| self.process_image_file(file, &resource_type) | ||
| } | ||
| ResourceType::Pdf => self.process_pdf_file(file), |
There was a problem hiding this comment.
image/webp is now detected as ResourceType::WebP and routed through process_image_file(), but create_image_xobject() currently only supports Png and Jpeg and returns an error for other variants. This means WebP resources will fail to process at runtime if they go through ResourceStreamer. Either add WebP support in the image embedding path, or don’t classify image/webp as a supported image type here until it’s implemented end-to-end.
| Ok(v.trunc() as i32) | ||
| } | ||
| fn visit_f64<E: serde::de::Error>(self, v: f64) -> Result<i32, E> { | ||
| Ok(v.trunc() as i32) |
There was a problem hiding this comment.
visit_f32/visit_f64 uses v.trunc() as i32. In Rust, float→int casts saturate (and NaN becomes 0), so out-of-range values like 1e30 will silently become i32::MAX (or MIN) rather than erroring, contradicting the expecting message (“representable as i32”). Consider explicitly rejecting non-finite values and performing a bounds check (e.g., truncate, then validate >= i32::MIN and <= i32::MAX) before converting, so malformed inputs don’t silently corrupt fields like seed/version.
| Ok(v.trunc() as i32) | |
| } | |
| fn visit_f64<E: serde::de::Error>(self, v: f64) -> Result<i32, E> { | |
| Ok(v.trunc() as i32) | |
| if !v.is_finite() { | |
| return Err(E::custom("expected finite float representable as i32")); | |
| } | |
| let t = v.trunc(); | |
| if t < i32::MIN as f32 || t > i32::MAX as f32 { | |
| return Err(E::custom("float is out of range for i32")); | |
| } | |
| Ok(t as i32) | |
| } | |
| fn visit_f64<E: serde::de::Error>(self, v: f64) -> Result<i32, E> { | |
| if !v.is_finite() { | |
| return Err(E::custom("expected finite float representable as i32")); | |
| } | |
| let t = v.trunc(); | |
| if t < i32::MIN as f64 || t > i32::MAX as f64 { | |
| return Err(E::custom("float is out of range for i32")); | |
| } | |
| Ok(t as i32) |
| #[test] | ||
| fn test_plot_all_assets() { | ||
| for file in list_duc_files() { | ||
| let duc_data = load_duc_file(&file); | ||
|
|
||
| let options = ConversionOptions { | ||
| scale: None, | ||
| mode: ConversionMode::Plot, | ||
| metadata_title: Some(file.clone()), | ||
| metadata_author: Some("DUC2PDF Test Suite".to_string()), | ||
| metadata_subject: Some("Asset plot conversion".to_string()), | ||
| background_color: Some("transparent".to_string()), | ||
| }; | ||
|
|
||
| match convert_duc_to_pdf_with_options(&duc_data, options) { | ||
| Ok(pdf_bytes) => { | ||
| validate_pdf_structure(&pdf_bytes, Path::new(&format!("memory:{file}"))); | ||
| println!("✅ {file}: plot OK, {} bytes", pdf_bytes.len()); | ||
| } | ||
| Err(e) => { | ||
| println!("⚠️ {file}: plot failed: {e}"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
test_plot_all_assets logs conversion errors but doesn’t fail the test, and will also trivially pass if the assets directory contains no .duc files. This makes the test ineffective in CI (regressions can be masked). Suggest either asserting each conversion succeeds, or at least failing when any conversion fails / when no assets are found (or marking the test #[ignore] if it’s meant to be a best-effort local smoke test).
| #[test] | ||
| fn test_parse_with_logging() { | ||
| init_logging(); | ||
|
|
||
| // Try to load a test DUC file from assets folder | ||
| let test_file_path = Path::new("assets/testing/duc-files/plot_elements.duc"); | ||
|
|
||
| if test_file_path.exists() { | ||
| match fs::read(test_file_path) { | ||
| Ok(data) => { | ||
| println!("Loaded test file: {} bytes", data.len()); | ||
|
|
||
| // Call the parse function to trigger the logging | ||
| match crate::parse::parse(&data) { | ||
| Ok(parsed) => { | ||
| println!("✅ Successfully parsed DUC file"); | ||
| println!("Layers count: {}", parsed.layers.len()); | ||
| } | ||
| Err(e) => { | ||
| println!("❌ Parse error: {}", e); | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| println!("❌ Failed to read test file: {}", e); | ||
| fn parse_all_assets() { | ||
| for path in all_duc_files() { | ||
| let buf = load(&path); | ||
| let state = parse::parse(&buf) | ||
| .unwrap_or_else(|e| panic!("parse {} failed: {e}", path.display())); | ||
|
|
||
| assert!(!state.version.is_empty(), "{}: version must not be empty", path.display()); | ||
| assert!(!state.source.is_empty(), "{}: source must not be empty", path.display()); | ||
| assert!(!state.elements.is_empty() || !state.layers.is_empty(), | ||
| "{}: must have elements or layers", path.display()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test module repeatedly iterates all_duc_files() and re-reads/re-parses every asset in multiple #[test]s. This can significantly increase CI time as the asset set grows. Consider caching the loaded bytes / parsed states once (e.g., via once_cell::sync::Lazy + Mutex/RwLock, or a helper that returns a pre-parsed vector) so each test can reuse the same data instead of doing full I/O + parse for every assertion suite.
📋 Release Preview
|
|
🎉 This PR is included in version 3.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.6.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.5.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
No description provided.