Change main stat preprocessing steps add an e2e test #2580
Conversation
…reshold, also add an e2e test
WalkthroughThis pull request introduces enhancements to the artifact image processing and testing infrastructure. The changes span multiple components, including end-to-end testing, image processing utilities, and the artifact editor component. The primary focus is on improving image preprocessing techniques, adding more robust testing for artifact image parsing, and enhancing the testability of the artifact management interface. Changes
Sequence DiagramsequenceDiagram
participant E2E as Cypress Test
participant UI as ArtifactEditor
participant Scanner as ArtifactScanner
participant ImgUtil as Image Processor
E2E->>UI: Navigate to Artifacts Page
E2E->>UI: Close Modal
E2E->>UI: Add New Artifact
UI->>Scanner: Upload Image
Scanner->>ImgUtil: Preprocess Image
ImgUtil-->>Scanner: Processed Image Data
Scanner->>UI: Parse Artifact Stats
E2E->>UI: Verify Main Stat
Possibly Related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -1,13 +1,35 @@ | |||
| import { getGreeting } from '../support/app.po' | |||
|
|
|||
| /* eslint-disable cypress/no-unnecessary-waiting */ | |||
There was a problem hiding this comment.
I know its better to create a map of some sort from filename to stats and use that. But, its a start, will raise another PR to fix the e2e
|
[sr-frontend] [Tue Dec 17 18:00:26 UTC 2024] - Deployed 52316aa to https://genshin-optimizer-prs.github.io/pr/2580/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Dec 17 18:00:50 UTC 2024] - Deployed 52316aa to https://genshin-optimizer-prs.github.io/pr/2580/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Wed Jan 8 02:15:52 UTC 2025] - Deployed 3c662da to https://genshin-optimizer-prs.github.io/pr/2580/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Jan 8 02:16:27 UTC 2025] - Deployed 3c662da to https://genshin-optimizer-prs.github.io/pr/2580/frontend (Takes 3-5 minutes after this completes to be available) [Wed Jan 8 03:04:41 UTC 2025] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
apps/frontend-e2e/src/e2e/app.cy.ts (1)
1-1: Consider removing the eslint disable comment.Instead of disabling the
cypress/no-unnecessary-waitingrule globally, consider using Cypress's built-in wait mechanisms that are more reliable.-/* eslint-disable cypress/no-unnecessary-waiting */libs/common/img-util/src/processing.ts (1)
129-142: Add parameter documentation and consider making parameters configurable.While the function is documented, the specific values used for blur radius, threshold level, etc., should be documented or made configurable.
/** * This function performs standard image preprocessing. Standard blur, * dilate, invert and threshold is done consecutively. * @param pixelData imagedata + * @param options Configuration object for preprocessing + * @param options.blurRadius Radius for Gaussian blur (default: 0.5) + * @param options.thresholdLevel Threshold level for binarization (default: 0.4) * @returns a preprocessed cloned image */ -export function preprocessImage(pixelData: ImageData) { +export function preprocessImage( + pixelData: ImageData, + options = { blurRadius: 0.5, thresholdLevel: 0.4 } +) { const imageClone = Uint8ClampedArray.from(pixelData.data) - blurARGB(imageClone, pixelData.width, pixelData.height, 0.5) + blurARGB(imageClone, pixelData.width, pixelData.height, options.blurRadius) dilate(imageClone, pixelData.width) invertColors(imageClone) - thresholdFilter(imageClone, 0.4) + thresholdFilter(imageClone, options.thresholdLevel) return new ImageData(imageClone, pixelData.width, pixelData.height) }libs/gi/ui/src/components/artifact/editor/index.tsx (1)
554-559: Consider enhancing test ID coverage for child elements.While adding
data-testid="main-stat"to the Box component is good, consider also adding test IDs to the interactive elements within this section (e.g., DropdownButton, MenuItem) to facilitate more granular e2e testing scenarios.Example additions:
<DropdownButton + data-testid="main-stat-dropdown" startIcon={ artifact?.mainStatKey ? ( <StatIcon statKey={artifact.mainStatKey} /> ) : undefined } // ... rest of the code > {artSlotMainKeys[slotKey].map((mainStatK) => ( <MenuItem + data-testid={`main-stat-option-${mainStatK}`} key={mainStatK} // ... rest of the code >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/frontend-e2e/assets/def_p_8_7.pngis excluded by!**/*.pngapps/frontend-e2e/assets/em_155.pngis excluded by!**/*.png
📒 Files selected for processing (4)
apps/frontend-e2e/src/e2e/app.cy.ts(1 hunks)libs/common/img-util/src/processing.ts(1 hunks)libs/gi/art-scanner/src/lib/processImg.ts(2 hunks)libs/gi/ui/src/components/artifact/editor/index.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
libs/common/img-util/src/processing.ts
[error] 204-204: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (3)
libs/gi/art-scanner/src/lib/processImg.ts (1)
243-243: Verify the impact of preprocessing changes.
The switch from bandPass to preprocessImage for header processing is a significant change. Ensure this change maintains or improves the accuracy of text extraction.
✅ Verification successful
The preprocessing change is an improvement in image processing
The switch from bandPass to preprocessImage for header processing is actually a more robust approach. The preprocessImage function applies a series of image processing operations including:
- Gaussian blur for noise reduction
- Dilation to enhance text features
- Color inversion
- Threshold filtering for binary image output
This combination is specifically designed for text extraction, making it more suitable for header processing than the simpler bandPass function.
Key observations:
bandPassis still used for other parts of the image where color-based filtering is needed- The preprocessing pipeline follows established computer vision practices for text extraction
- The implementation is based on well-tested image processing algorithms from p5.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any related test files or image processing configurations
# that might need updates due to the preprocessing change.
# Look for test files related to image processing
fd -e test.ts -e spec.ts . | xargs rg -l "bandPass|preprocessImage"
# Look for configuration files that might contain image processing parameters
fd -e config.ts -e json . | xargs rg -l "bandPass|preprocessImage"
Length of output: 138
Script:
#!/bin/bash
# Let's try a broader search to understand the usage and testing of these image processing functions
# Search for bandPass and preprocessImage function definitions and usages
rg -l "function\s+(bandPass|preprocessImage)" --type ts
# Search for test files containing any image processing related terms
rg -l "test|describe|it.*(\bimage\b|\bpreprocess\b|\bheader\b|\btext extraction\b)" --type ts
# Look for any image processing related configuration or constants
rg -l "(\bimage\b|\bpreprocess\b|\bheader\b)\s*(:|=)\s*{" --type ts
Length of output: 5378
Script:
#!/bin/bash
# Let's examine the image processing functions in the identified files
# Check the implementation of preprocessImage and bandPass
rg -A 10 "function\s+(preprocessImage|bandPass)" libs/common/img-util/src/processing.ts
# Look for tests specifically related to image preprocessing
rg -A 10 "(test|describe|it).*(\bpreprocess|\bimage)" libs/common/img-util/src/processing.ts
# Check for any usage of these functions in the art-scanner module
rg "(preprocessImage|bandPass)" libs/gi/art-scanner/src/
Length of output: 2990
libs/gi/ui/src/components/artifact/editor/index.tsx (2)
431-431: LGTM! Test ID added for modal wrapper.
The addition of data-testid="artifact-editor" aligns with the PR's objective of adding e2e tests.
Line range hint 431-559: Verify test ID usage in e2e tests.
Let's verify that these newly added test IDs are being utilized in the e2e tests mentioned in the PR objectives.
✅ Verification successful
Test IDs are properly utilized in e2e tests
The test IDs artifact-editor and main-stat are actively being used in the e2e test file apps/frontend-e2e/src/e2e/app.cy.ts for verifying artifact editor functionality, including main stat value and text assertions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the new test IDs in e2e test files
echo "Searching for test ID usage in e2e tests..."
rg -l "artifact-editor|main-stat" "apps/frontend-e2e/src/e2e"
Length of output: 194
Script:
#!/bin/bash
# Let's check the actual usage context of these test IDs in the e2e test file
rg -A 5 -B 5 "artifact-editor|main-stat" "apps/frontend-e2e/src/e2e/app.cy.ts"
Length of output: 865
| cy.contains('button', 'Clear').click() | ||
| cy.wait(500) | ||
|
|
||
| // Function helper example, see `../support/app.po.ts` file | ||
| getGreeting().contains('Welcome frontend') | ||
| cy.contains('label', 'Upload Screenshot').selectFile('assets/def_p_8_7.png') | ||
| cy.wait(5000) | ||
| cy.contains('button', 'Add Artifact', { timeout: 60000 }).should( | ||
| 'not.be.disabled' | ||
| ) | ||
| cy.get( | ||
| '[data-testid="artifact-editor"] [data-testid="main-stat"] button' | ||
| ).should('have.text', 'DEF%') | ||
| cy.get( | ||
| '[data-testid="artifact-editor"] [data-testid="main-stat"] p' | ||
| ).should('have.text', '8.7%') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated test logic into a custom command.
The second image test repeats the same steps as the first one. Consider creating a custom Cypress command to reduce duplication.
// In commands.ts
Cypress.Commands.add('verifyArtifactMainStat', (imagePath: string, expectedStat: string, expectedValue: string) => {
cy.contains('button', 'Clear').click()
cy.contains('label', 'Upload Screenshot').selectFile(imagePath)
cy.get('[data-testid="artifact-editor"]').should('exist')
cy.get('button').contains('Add Artifact').should('not.be.disabled')
cy.get('[data-testid="artifact-editor"] [data-testid="main-stat"] button').should('have.text', expectedStat)
cy.get('[data-testid="artifact-editor"] [data-testid="main-stat"] p').should('have.text', expectedValue)
})
// In test
it('should correctly parse main stats from artifact screenshots', () => {
cy.visit('/#/artifacts')
cy.get('.MuiModal-root [data-testid="CloseIcon"]').click()
cy.verifyArtifactMainStat('assets/em_155.png', 'Elemental Mastery', '155')
cy.verifyArtifactMainStat('assets/def_p_8_7.png', 'DEF%', '8.7%')
})| it('should test artifacts images to check if main stat is being parsed correctly', () => { | ||
| cy.visit('/#/artifacts', { timeout: 600000 }) | ||
| cy.get('.MuiModal-root [data-testid="CloseIcon"]').click() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test description and improve timeout handling.
The test description could be more specific, and the timeout should be moved to Cypress configuration.
- it('should test artifacts images to check if main stat is being parsed correctly', () => {
- cy.visit('/#/artifacts', { timeout: 600000 })
+ it('should correctly parse main stats from artifact screenshots', () => {
+ cy.visit('/#/artifacts')Committable suggestion skipped: line range outside the PR's diff.
| cy.contains('label', 'Upload Screenshot').selectFile('assets/em_155.png') | ||
| cy.wait(5000) | ||
| cy.contains('button', 'Add Artifact', { timeout: 60000 }).should( | ||
| 'not.be.disabled' | ||
| ) | ||
| cy.get( | ||
| '[data-testid="artifact-editor"] [data-testid="main-stat"] button' | ||
| ).should('have.text', 'Elemental Mastery') | ||
| cy.get( | ||
| '[data-testid="artifact-editor"] [data-testid="main-stat"] p' | ||
| ).should('have.text', '155') |
There was a problem hiding this comment.
Replace fixed waits with proper assertions.
Using fixed wait times can make tests flaky. Instead, use Cypress's built-in retry-ability and assertions.
- cy.wait(5000)
+ cy.get('[data-testid="artifact-editor"]').should('exist')
+ cy.get('@addArtifact').should('not.be.disabled')Committable suggestion skipped: line range outside the PR's diff.
| // internal kernel stuff for the gaussian blur filter | ||
| let blurRadius: number | ||
| let blurKernelSize: number | ||
| let blurKernel: Int32Array | ||
| let blurMult: Array<Int32Array> | ||
|
|
||
| // from https://github.com/processing/p5.js/blob/main/src/image/filters.js | ||
| function buildBlurKernel(r: number) { | ||
| let radius = (r * 3.5) | 0 | ||
| radius = radius < 1 ? 1 : radius < 248 ? radius : 248 | ||
|
|
||
| if (blurRadius !== radius) { | ||
| blurRadius = radius | ||
| blurKernelSize = (1 + blurRadius) << 1 | ||
| blurKernel = new Int32Array(blurKernelSize) | ||
| blurMult = new Array(blurKernelSize) | ||
| for (let l = 0; l < blurKernelSize; l++) { | ||
| blurMult[l] = new Int32Array(256) | ||
| } | ||
|
|
||
| let bki: number | ||
| let bm, bmi: Int32Array | ||
|
|
||
| for (let i = 1, radiusi = radius - 1; i < radius; i++) { | ||
| blurKernel[radius + i] = blurKernel[radiusi] = bki = radiusi * radiusi | ||
| bm = blurMult[radius + i] | ||
| bmi = blurMult[radiusi--] | ||
| for (let j = 0; j < 256; j++) { | ||
| bm[j] = bmi[j] = bki * j | ||
| } | ||
| } | ||
| const bk = (blurKernel[radius] = radius * radius) | ||
| bm = blurMult[radius] | ||
|
|
||
| for (let k = 0; k < 256; k++) { | ||
| bm[k] = bk * k | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider encapsulating blur kernel state.
The blur kernel implementation uses global variables which could lead to issues in concurrent processing. Consider encapsulating this state within a class.
class GaussianBlur {
private blurRadius: number;
private blurKernelSize: number;
private blurKernel: Int32Array;
private blurMult: Array<Int32Array>;
constructor() {
this.blurRadius = 0;
this.blurKernelSize = 0;
this.blurKernel = new Int32Array(0);
this.blurMult = [];
}
buildKernel(r: number) {
// ... existing buildBlurKernel logic ...
}
blur(pixels: Uint8ClampedArray, width: number, height: number, radius: number) {
// ... existing blurARGB logic using class properties ...
}
}🧰 Tools
🪛 Biome (1.9.4)
[error] 204-204: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| bm[j] = bmi[j] = bki * j | ||
| } | ||
| } | ||
| const bk = (blurKernel[radius] = radius * radius) |
There was a problem hiding this comment.
Fix assignment in expression.
The static analysis tool correctly flags an assignment within an expression. This should be split into separate statements for better readability.
- const bk = (blurKernel[radius] = radius * radius)
+ const bk = radius * radius
+ blurKernel[radius] = bk📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bk = (blurKernel[radius] = radius * radius) | |
| const bk = radius * radius | |
| blurKernel[radius] = bk |
🧰 Tools
🪛 Biome (1.9.4)
[error] 204-204: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
Describe your changes
This change aims to replace the bandpass preprocessing steps to a standard blur->dilate->invert->threshold preprocessing for the cropped artifact header.
Additionally, it adds an initial E2E test for testing artifacts.
Original processed header
New preprocessed header
Issue or discord link
#2368
Testing/validation
Ran few artifact images from my own genshin game.
Running E2e test
Ensure the local dev server is running on
http://localhost:4200/nx serve frontendRun cypress test
npx nx open-cypress frontend-e2e --config="baseUrl=http://localhost:4200/"Click on E2E testing + select a browser to open a cypress dashboard in that browser.
Click on
src\e2e\app.cy.tsto run the e2e test.Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
I have made corresponding changes to README or wiki.For front-end changes, I have updated the corresponding English translations.yarn run mini-cilocally to validate format and lint.If I have added a new library or app, I have updated the deployment scripts to ignore changes as neededSummary by CodeRabbit
New Features
Improvements
ArtifactEditorcomponent for improved testability and maintainability.