Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Core (Tests): Add unit test for image loading #258

Merged
merged 2 commits into from
Jan 19, 2025
Merged

Core (Tests): Add unit test for image loading #258

merged 2 commits into from
Jan 19, 2025

Conversation

kaixiong
Copy link
Member

@kaixiong kaixiong commented Feb 6, 2023

No description provided.

@kaixiong kaixiong requested a review from hartwork February 6, 2023 15:16
@kaixiong kaixiong self-assigned this Feb 6, 2023
@kaixiong kaixiong changed the title Core (Tests): Add unit test for image loading. Core (Tests): Add unit test for image loading Feb 6, 2023
hartwork
hartwork reviewed Feb 6, 2023
View reviewed changes
Copy link
Member

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@kaixiong I like this pull request but it seems to have some not-terminating or infinite-loop problem: Multiple CI tasks are stuck during tests execution. Any ideas?

@hartwork
Copy link
Member

hartwork commented Feb 6, 2023

@kaixiong update: I figured now that that's what pull requests #259 is fixing. Let's merge #259 then and rebase #258 onto latest master and then CI should be all green with some luck.

@hartwork
Copy link
Member

hartwork commented Feb 6, 2023

PS: I have canceled the related CI runs for now so that runners can pick up other jobs again.

@kaixiong
Copy link
Member Author

kaixiong commented Feb 7, 2023

@hartwork, it's fixed!

There are still tests for 8-bit and 16-bit images to come. Once this PR is completed and merged, the functions developed here will be used to validate the PNG save function (#206) by means of round-tripping.

@hartwork
Copy link
Member

hartwork commented Feb 7, 2023

@hartwork, it's fixed!

@kaixiong it's rebased on 7d536d1 by now, I see. Nice, I won't cancel it again then, good to know! 😄

There are still tests for 8-bit and 16-bit images to come. Once this PR is completed and merged, the functions developed here will be used to validate the PNG save function (#206) by means of round-tripping.

Sounds good! 👍

@kaixiong kaixiong force-pushed the image-testing branch 3 times, most recently from 7c461b8 to d9611dd Compare January 17, 2025 06:29
@kaixiong kaixiong marked this pull request as ready for review January 17, 2025 06:44
@kaixiong kaixiong requested a review from hartwork January 17, 2025 06:45
hartwork
hartwork approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@kaixiong I found one thing:

Commit "Core (Tests): Add test for BMP image loading." seems to be doing to separate things:

  • stop load_raw_indexed_image from converting the image to VISUAL_VIDEO_DEPTH_24BIT
  • add BMP loading tests on top

I believe splitting this in two would yield two almost-trivial-to-review commits for the developers after you and me (and future versions of ourselves) while currently the splitting needs to be done in the reviewer head and is "actual work" and takes while. What do you think?

@kaixiong
Copy link
Member Author

@hartwork Oh dear, it looks like I missed that change in load_raw_indexed_image when cleaning up the commits. I agree with you, let me fix that.

@kaixiong
Copy link
Member Author

@hartwork I did some touching up and divided the change into two distinct commits for simplicity, one for PNG and one for BMP.

hartwork
hartwork approved these changes Jan 18, 2025
View reviewed changes
Copy link
Member

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@kaixiong thanks for the adjustments! 👍

@hartwork hartwork added this to the 0.5.0_alpha1 milestone Jan 18, 2025
@kaixiong kaixiong merged commit 6359c3e into master Jan 19, 2025
6 checks passed
@kaixiong kaixiong deleted the image-testing branch January 19, 2025 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hartwork hartwork hartwork approved these changes

Assignees

@kaixiong kaixiong

Labels
enhancement porting: not needed
Projects
None yet
Milestone
0.5.0_alpha1
Development

Successfully merging this pull request may close these issues.
2 participants
@kaixiong @hartwork