Skip to content

[chore] Adding a ton of unit tests #126

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

Merged
merged 1 commit into from
Jun 10, 2025
Merged

[chore] Adding a ton of unit tests #126

merged 1 commit into from
Jun 10, 2025

Conversation

blefaudeux
Copy link
Contributor

@blefaudeux blefaudeux commented Jun 8, 2025

Making sure the behavior is locked in place, so that it's easier for others to contribute.

Code significantly LLM generated (like 99%, still reviewing myself), kind of a first for me but I figured that was a good test case. Using Claude Code, to be transparent I gave it a shot on this task (speed up the image processing) at the time but it didn't do much good (1% speed up (: ) and I did the PR as a human :) For unit tests it seems pretty valid.

@blefaudeux blefaudeux requested a review from photoroman June 8, 2025 20:14
@blefaudeux blefaudeux requested a review from tarek-ayed June 8, 2025 20:14
@blefaudeux blefaudeux marked this pull request as draft June 8, 2025 20:31
@blefaudeux blefaudeux force-pushed the ben/more_unit_tests branch from 5934c92 to 6f17a6d Compare June 8, 2025 20:36
@blefaudeux blefaudeux force-pushed the ben/more_unit_tests branch from 6f17a6d to 499cfb9 Compare June 8, 2025 20:41
@blefaudeux blefaudeux force-pushed the ben/more_unit_tests branch from 499cfb9 to 7df5af5 Compare June 8, 2025 20:45
@blefaudeux blefaudeux requested a review from Copilot June 8, 2025 20:46
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a comprehensive suite of unit tests to lock in current behavior across image processing, sample pulling, struct serialization, and file enumeration, plus a minor version bump and dev‐dependency addition.

  • Extensive #[cfg(test)] modules added in four source files, covering success and error paths.
  • Introduced #[serde(default)] on several ImageTransformConfig fields and added precondition asserts in its constructor.
  • Bumped Cargo package version and added tempfile under [dev-dependencies].

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/worker_files.rs Added image load tests and pull_samples/async_pull_samples tests
src/structs.rs Added constructor and serialization tests for payloads and configs
src/image_processing.rs Added #[serde(default)], validation asserts, and image‐payload tests
src/generator_files.rs Added tests for hash, file enumeration, and SourceFileConfig
Cargo.toml Bumped version to 2025.6.3 and added tempfile under dev-deps
Comments suppressed due to low confidence (3)

src/image_processing.rs:31

  • The image_to_rgb8 field lacks #[serde(default)], so deserializing JSON that omits it will fail, even though tests expect it to default to false. Add #[serde(default)] above this field.
pub image_to_rgb8: bool,

src/image_processing.rs:55

  • [nitpick] build_image_size_list may insert duplicates when aspect ratios overlap. Consider deduplicating and/or sorting the output to ensure consistent, deterministic size lists.
img_sizes.push((img_w, img_h));

src/worker_files.rs:167

  • [nitpick] This helper is duplicated in multiple test modules; consider extracting shared test utilities (e.g., image creation) into a common tests/utils.rs to reduce code duplication.
fn create_test_image(path: &std::path::Path) {

@blefaudeux blefaudeux force-pushed the ben/more_unit_tests branch from 7df5af5 to 8cdda02 Compare June 8, 2025 20:57
@blefaudeux blefaudeux changed the title [WIP] Adding a ton of unit tests [chore] Adding a ton of unit tests Jun 8, 2025
@blefaudeux blefaudeux marked this pull request as ready for review June 8, 2025 21:26
@blefaudeux
Copy link
Contributor Author

not changing any functionality, guess is that this is good for everyone, saving time

@blefaudeux blefaudeux merged commit 303c04f into main Jun 10, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant