Move scanActualColors to JupiterTestBase and remove duplicates

[manuelsblanco]

User description

What does this PR do?

Moves the shared test utility scanActualColors(BufferedImage, int, int) to JupiterTestBase and deletes the duplicate implementations in:

• org.openqa.selenium.TakesScreenshotTest
• org.openqa.selenium.firefox.TakesFullPageScreenshotTest

Centralizing this logic removes duplication, makes maintenance easier, and ensures all screenshot tests rely on the same well-documented implementation.

Why?

• DRY: Two nearly identical copies were diverging risks for future changes.
• Discoverability: Test helpers belong in a common base (JupiterTestBase) where other JUnit 5 tests can reuse them.
• Consistency: A single, asserted and documented implementation for color scanning.

How (Implementation Notes)

• Added protected final Set<String> scanActualColors(BufferedImage image, int stepX, int stepY) to JupiterTestBase.
• Copied the existing logic verbatim, preserved assertions and failure handling.
• Removed the private duplicates and updated imports/usages accordingly.
• Left a short comment in the affected tests pointing to the base method.

Tests

• No functional test changes required. Existing screenshot tests continue to pass using the shared helper.
• Verified compilation and usages in both affected classes.

Types of changes

• Refactor (non-breaking change that reduces duplication; no behavior change)

Risks & Trade-offs

• Low: Method visibility is protected + final to encourage reuse while preventing overrides that could fragment behavior across tests.

Follow-ups (optional)

• Consider adding small input preconditions (e.g., stepX > 0, stepY > 0) and a short doc comment on sampling strategy.

---

PR Type

Enhancement

---

Description

• Centralizes scanActualColors() utility method in JupiterTestBase

• Removes duplicate implementations from two test classes

• Reduces code duplication and improves maintainability

• Method marked protected final to encourage reuse while preventing overrides

---

Diagram Walkthrough

flowchart LR
  A["TakesScreenshotTest<br/>scanActualColors duplicate"] -->|removed| B["JupiterTestBase<br/>scanActualColors shared"]
  C["TakesFullPageScreenshotTest<br/>scanActualColors duplicate"] -->|removed| B
  B -->|inherited| A
  B -->|inherited| C

File Walkthrough

	Relevant files
Refactoring	TakesScreenshotTest.java Remove duplicate scanActualColors method                                  java/test/org/openqa/selenium/TakesScreenshotTest.java Removed private scanActualColors() method (38 lines) Method now inherited from JupiterTestBase No functional changes to test logic +0/-37    TakesFullPageScreenshotTest.java Remove duplicate scanActualColors and clean imports            java/test/org/openqa/selenium/firefox/TakesFullPageScreenshotTest.java Removed private scanActualColors() method (38 lines) Removed unused Raster import Updated class javadoc comment to reference parent package tests Method now inherited from JupiterTestBase +1/-40
Enhancement	JupiterTestBase.java Add shared scanActualColors utility method                              java/test/org/openqa/selenium/testing/JupiterTestBase.java Added protected final scanActualColors() method with full implementation Added necessary imports: BufferedImage, Raster, Set, TreeSet, assertThat, fail Method scans image at grid intervals and returns hex color strings Includes validation and error handling with descriptive javadoc +44/-0

---

[qodo-code-review[bot]]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Input validation Description: The method scanActualColors uses a broad try/catch that converts any exception into a generic test failure, potentially masking specific errors like invalid step sizes (e.g., zero or negative), which could lead to infinite loops or runtime exceptions if called improperly. JupiterTestBase.java [134-162] Referred Code protected final Set<String> scanActualColors(BufferedImage image, final int stepX, final int stepY) { Set<String> colors = new TreeSet<>(); try { int height = image.getHeight(); int width = image.getWidth(); assertThat(width > 0).isTrue(); assertThat(height > 0).isTrue(); Raster raster = image.getRaster(); for (int i = 0; i < width; i = i + stepX) { for (int j = 0; j < height; j = j + stepY) { String hex = String.format( "#%02x%02x%02x", (raster.getSample(i, j, 0)), (raster.getSample(i, j, 1)), (raster.getSample(i, j, 2))); colors.add(hex); } } ... (clipped 8 lines)
Ticket Compliance
⚪	🎫 No ticket provided [ ] Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logging: The added helper method scanActualColors performs image processing without any audit logging, but as this is test code and not a critical system action, applicability of audit requirements is unclear. Referred Code protected final Set<String> scanActualColors(BufferedImage image, final int stepX, final int stepY) { Set<String> colors = new TreeSet<>(); try { int height = image.getHeight(); int width = image.getWidth(); assertThat(width > 0).isTrue(); assertThat(height > 0).isTrue(); Raster raster = image.getRaster(); for (int i = 0; i < width; i = i + stepX) { for (int j = 0; j < height; j = j + stepY) { String hex = String.format( "#%02x%02x%02x", (raster.getSample(i, j, 0)), (raster.getSample(i, j, 1)), (raster.getSample(i, j, 2))); colors.add(hex); } } ... (clipped 8 lines)
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Broad exception catch: scanActualColors catches a generic Exception and converts it to a test failure without validating stepX/stepY > 0 or guarding against zero/negative steps that could cause infinite loops. Referred Code protected final Set<String> scanActualColors(BufferedImage image, final int stepX, final int stepY) { Set<String> colors = new TreeSet<>(); try { int height = image.getHeight(); int width = image.getWidth(); assertThat(width > 0).isTrue(); assertThat(height > 0).isTrue(); Raster raster = image.getRaster(); for (int i = 0; i < width; i = i + stepX) { for (int j = 0; j < height; j = j + stepY) { String hex = String.format( "#%02x%02x%02x", (raster.getSample(i, j, 0)), (raster.getSample(i, j, 1)), (raster.getSample(i, j, 2))); colors.add(hex); } } ... (clipped 8 lines)
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Missing input checks: scanActualColors does not validate stepX and stepY for positive non-zero values before using them as loop increments, which could lead to non-terminating loops or errors. Referred Code protected final Set<String> scanActualColors(BufferedImage image, final int stepX, final int stepY) { Set<String> colors = new TreeSet<>(); try { int height = image.getHeight(); int width = image.getWidth(); assertThat(width > 0).isTrue(); assertThat(height > 0).isTrue(); Raster raster = image.getRaster(); for (int i = 0; i < width; i = i + stepX) { for (int j = 0; j < height; j = j + stepY) { String hex = String.format( "#%02x%02x%02x", (raster.getSample(i, j, 0)), (raster.getSample(i, j, 1)), (raster.getSample(i, j, 2))); colors.add(hex); } } ... (clipped 8 lines)
[ ] Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review[bot]]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent infinite loops with input validation Add a precondition check to the scanActualColors method to ensure stepX and stepY are positive, preventing potential infinite loops. java/test/org/openqa/selenium/testing/JupiterTestBase.java [134-162] protected final Set<String> scanActualColors(BufferedImage image, final int stepX, final int stepY) { + if (stepX <= 0 || stepY <= 0) { + throw new IllegalArgumentException("Step values must be positive."); + } Set<String> colors = new TreeSet<>(); try { int height = image.getHeight(); int width = image.getWidth(); assertThat(width > 0).isTrue(); assertThat(height > 0).isTrue(); Raster raster = image.getRaster(); for (int i = 0; i < width; i = i + stepX) { for (int j = 0; j < height; j = j + stepY) { String hex = String.format( "#%02x%02x%02x", (raster.getSample(i, j, 0)), (raster.getSample(i, j, 1)), (raster.getSample(i, j, 2))); colors.add(hex); } } } catch (Exception e) { fail("Unable to get actual colors from screenshot: " + e.getMessage()); } assertThat(colors).isNotEmpty(); return colors; } [ ] Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential infinite loop if stepX or stepY are not positive, which would cause tests to hang. Adding input validation makes the new shared method more robust.	Medium
General	Improve color extraction from images Replace raster.getSample() with the more robust image.getRGB() to ensure correct color extraction regardless of the image's color model. java/test/org/openqa/selenium/testing/JupiterTestBase.java [143-154] -Raster raster = image.getRaster(); for (int i = 0; i < width; i = i + stepX) { for (int j = 0; j < height; j = j + stepY) { + int rgb = image.getRGB(i, j); String hex = - String.format( - "#%02x%02x%02x", - (raster.getSample(i, j, 0)), - (raster.getSample(i, j, 1)), - (raster.getSample(i, j, 2))); + String.format("#%02x%02x%02x", (rgb >> 16) & 0xFF, (rgb >> 8) & 0xFF, rgb & 0xFF); colors.add(hex); } } [ ] Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that using raster.getSample is not robust. Switching to image.getRGB makes the color sampling logic independent of the image's underlying color model, improving the method's reliability.	Medium
High-level	Use a dedicated image utility class Instead of adding the scanActualColors method to the JupiterTestBase class, create a new dedicated utility class, such as ImageTestUtils, to hold this and other image-related helper methods. This improves code organization and keeps the base class focused on its core responsibilities. Examples: java/test/org/openqa/selenium/testing/JupiterTestBase.java [126-162] /** * Get colors from image from each point at grid defined by stepX/stepY. * * @param image - image * @param stepX - interval in pixels b/w point in X dimension * @param stepY - interval in pixels b/w point in Y dimension * @return set of colors in string hex presentation */ protected final Set<String> scanActualColors(BufferedImage image, final int stepX, final int stepY) { Set<String> colors = new TreeSet<>(); ... (clipped 27 lines) Solution Walkthrough: Before: // java/test/org/openqa/selenium/testing/JupiterTestBase.java public abstract class JupiterTestBase { // ... driver and environment setup ... protected final Set<String> scanActualColors(BufferedImage image, ...) { // ... implementation to scan image colors ... } } // java/test/org/openqa/selenium/TakesScreenshotTest.java public class TakesScreenshotTest extends JupiterTestBase { @Test void testSomething() { // ... Set<String> actualColors = scanActualColors(image, 10, 10); // ... } } After: // java/test/org/openqa/selenium/testing/ImageTestUtils.java public final class ImageTestUtils { private ImageTestUtils() {} public static Set<String> scanActualColors(BufferedImage image, ...) { // ... implementation to scan image colors ... } } // java/test/org/openqa/selenium/testing/JupiterTestBase.java public abstract class JupiterTestBase { // ... driver and environment setup ... // scanActualColors method is removed } // java/test/org/openqa/selenium/TakesScreenshotTest.java public class TakesScreenshotTest extends JupiterTestBase { @Test void testSomething() { // ... Set<String> actualColors = ImageTestUtils.scanActualColors(image, 10, 10); // ... } } Suggestion importance[1-10]: 5 __ Why: The suggestion proposes a valid design alternative that improves long-term code organization by separating concerns, though the PR's current approach is also a reasonable and common solution for code deduplication.	Low
[ ] Update

[manuelsblanco]

If the point of this PR is to consolidate and remove duplicate code, the rest of the duplicate screenshot-related methods in ./firefox/TakesFullPageScreenshotTest.java should also be moved. It makes no sense to just move one method while leaving the others.

Also, JupiterTestBase.java contains no other similar helper methods. These should go in ./testing/TestUtilities.java or in its own class/file under ./testing/.

You're absolutely right — I also noticed there are several other duplicated screenshot-related methods.

Before moving forward and removing more of that duplicated code, I wanted to make sure the team agrees with this approach. My goal with this PR was to start small and keep the changes easy to review, rather than submitting a large PR affecting multiple classes at once.

Once there's alignment on this direction, I can continue cleaning up the remaining duplicated methods in subsequent PRs.

[manuelsblanco]

@manuelsblanco Also, the majority of your PR's have either been rejected (for being unnecessary or breaking things) or just closed with unmerged commits. I'm not sure what your goal is, but you have wasted a significant amount of reviewers/maintainers time over the last 2 years.

I don’t really keep track of which of my PRs were accepted or not — I contribute because I believe in the project, not to waste anyone’s time.

I honestly don’t understand the need for such a passive-aggressive message. Is this meant as an invitation for me to stop contributing? I don’t think this is the right way to respond to a PR.

Maybe my proposed improvement isn’t good enough for you, but I did it with the best intentions and genuine willingness to help the project move forward.

[cgoldberg]

I can continue cleaning up the remaining duplicated methods in subsequent PRs

The screenshot-related methods are all related and small. Just add them to a single PR.