Skip to content

Conversation

@AntonisDevStuff
Copy link

Additional tests for all from_* class methods of Color to close #3491

@AntonisDevStuff AntonisDevStuff requested a review from a team as a code owner November 4, 2025 19:24
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

📝 Walkthrough

Walkthrough

This PR adds test coverage for Color instance methods that mirror classmethod conversions. Tests verify that calling from_* methods on Color instances returns expected color component tuples, validating existing functionality without introducing API changes.

Changes

Cohort / File(s) Change Summary
Color instance method tests
test/color_test.py
Added tests for instance method calls to from_cmy, from_hsva, from_hsla, from_i1i2i3, from_normalized, and from_hex, each verifying that instance method results match their classmethod counterparts.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add more tests for color class methods' accurately reflects the main change of adding test coverage for color class methods.
Description check ✅ Passed The description appropriately states the purpose of the PR and references the linked issue #3491 that requests additional test coverage.
Linked Issues check ✅ Passed The changes add tests for all from_* class methods (from_cmy, from_hsva, from_hsla, from_i1i2i3, from_normalized, from_hex) to verify they work when called on instances and produce expected results, fulfilling issue #3491 requirements.
Out of Scope Changes check ✅ Passed All changes are test-only additions to validate existing functionality; no out-of-scope modifications to core logic or unrelated areas were introduced.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
test/color_test.py (1)

790-790: Tests validate instance methods correctly, but consider testing with varied initial colors.

The instance method tests work correctly. However, to better align with issue #3491's requirement that "the original value of the instance should not affect the resulting color", consider testing with different initial colors beyond (0, 0, 0, 0). The test_from_hex method demonstrates this pattern well by using varied initial colors like (63, 12, 83) and (52, 31, 8, 255).

Example for test_from_cmy:

 def test_from_cmy(self):
     cmy = pygame.Color.from_cmy(0.5, 0.5, 0.5)
     cmy_tuple = pygame.Color.from_cmy((0.5, 0.5, 0.5))
     cmy_instance = pygame.Color(0, 0, 0, 0).from_cmy(0.5, 0.5, 0.5)
+    cmy_instance2 = pygame.Color(255, 128, 64, 200).from_cmy(0.5, 0.5, 0.5)

     expected_cmy = (127, 127, 127)

     self.assertEqual(expected_cmy, cmy)
     self.assertEqual(expected_cmy, cmy_tuple)
     self.assertEqual(expected_cmy, cmy_instance)
+    self.assertEqual(expected_cmy, cmy_instance2)

Also applies to: 796-796, 808-808, 814-814, 826-826, 832-832, 844-844, 850-850, 860-860, 866-866

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a966d54 and 8de7e98.

📒 Files selected for processing (1)
  • test/color_test.py (7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T20:22:31.010Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.

Applied to files:

  • test/color_test.py
🧬 Code graph analysis (1)
test/color_test.py (1)
buildconfig/stubs/pygame/color.pyi (12)
  • Color (12-94)
  • from_cmy (54-54)
  • from_cmy (57-57)
  • from_hsva (60-60)
  • from_hsva (63-63)
  • from_hsla (66-66)
  • from_hsla (69-69)
  • from_i1i2i3 (72-72)
  • from_i1i2i3 (75-75)
  • from_normalized (78-78)
  • from_normalized (81-81)
  • from_hex (83-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
  • GitHub Check: x86_64
  • GitHub Check: Pyodide build
  • GitHub Check: build (macos-14)
  • GitHub Check: build (windows-latest)
  • GitHub Check: Debian (Bookworm - 12) [s390x]
  • GitHub Check: dev-check
  • GitHub Check: x86
  • GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
  • GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
  • GitHub Check: AMD64
  • GitHub Check: msys2 (ucrt64, ucrt-x86_64)
  • GitHub Check: Debian (Bookworm - 12) [armv6]
  • GitHub Check: Debian (Bookworm - 12) [armv7]
  • GitHub Check: Debian (Bookworm - 12) [ppc64le]
  • GitHub Check: msys2 (clang64, clang-x86_64)
  • GitHub Check: aarch64
  • GitHub Check: x86_64
  • GitHub Check: build (ubuntu-22.04)
  • GitHub Check: i686
  • GitHub Check: msys2 (mingw64, x86_64)
🔇 Additional comments (1)
test/color_test.py (1)

878-882: Excellent test coverage for from_hex instance method!

These tests effectively demonstrate that the original instance value doesn't affect the from_hex result by using varied initial colors. This pattern aligns well with the requirements from issue #3491.

Also applies to: 893-896

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🎉

Copy link
Contributor

@aatle aatle left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing.

@aatle aatle added color pygame.color tests tests (module) labels Nov 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

color pygame.color tests tests (module)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tests: check color classmethods on instances

3 participants