Test/expand tests #389
Test/expand tests #389
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis PR significantly refactors and expands the test suite for the ecc-utils-design form component. The changes focus on improving test organization, readability, and maintainability by introducing a new test structure and helper classes for handling different form field types. Class diagram for EccUtilsDesignForm changesclassDiagram
class EccUtilsDesignForm {
+handleTusFileUpload(e: Event, field: Field): Promise<Record<string, string> | null>
+renderInputTemplate(field: Field, path: string): TemplateResult
+renderArrayTemplate(field: Field, path: string): TemplateResult
+renderGroupTemplate(field: Field, path: string): TemplateResult
+renderErrorTemplate(): TemplateResult
+renderSuccessTemplate(): TemplateResult
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @SalihuDickson - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const file = (e.target as HTMLInputElement).files?.[0]; | ||
|
|
||
| if (!file) { | ||
| console.error("No file selected for upload."); | ||
| return; | ||
| return null; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
issue: File upload error handling needs improvement
The handleTusFileUpload method silently fails and returns null on error. Consider adding proper error state handling and user feedback through the form's error display mechanism.
| text = "test value" | ||
| ) { | ||
| public async fill(text = "test value") { | ||
| if (this.el.getAttribute("disable")) return; |
There was a problem hiding this comment.
issue: Use standard 'disabled' attribute instead of custom 'disable'
The code uses a non-standard 'disable' attribute. Use the standard HTML 'disabled' attribute instead for better compatibility and consistency.
| } else { | ||
| _.set(this.form, path, value); | ||
| _.set(this.form, path, value.trim()); |
There was a problem hiding this comment.
suggestion (performance): Unnecessary string trimming on every input change
Trimming the input value on every change is inefficient. Consider moving the trim operation to the blur event handler or form submission instead.
| _.set(this.form, path, value.trim()); | |
| _.set(this.form, path, value); | |
| this.addEventListener('blur', () => { | |
| _.set(this.form, path, value.trim()); | |
| }, { once: true }); |
| component: ComponentType; | ||
| constructor(component: ComponentType) { | ||
| this.component = component; | ||
| export default class Field { |
There was a problem hiding this comment.
issue (complexity): Consider consolidating the class hierarchy into a single class with utility functions for better code organization
The inheritance hierarchy adds unnecessary complexity. Consider simplifying to a single class using utility functions:
export class TestComponent {
private el: ParentElement;
private component: ComponentType;
constructor(el: Element | LitElement, instance: InstanceType = "element") {
if (instance === "litElement") {
this.component = el as LitElement;
this.el = el.shadowRoot!;
} else {
this.el = el;
}
}
private getElement(label?: string, testId?: string, all = false) {
// Existing _getFields logic
}
// Utility functions instead of inheritance
private async handleElementAction(el: HTMLElement, action: () => void) {
if (el.getAttribute("disable")) return;
action();
await this.component?.updateComplete;
}
public async fillInput(el: HTMLInputElement, text = "test value") {
return this.handleElementAction(el, () => {
el.value = text;
el.dispatchEvent(new Event("sl-input"));
});
}
public async selectOption(el: HTMLSelectElement, label: string | number) {
return this.handleElementAction(el, () => {
// Existing select logic
});
}
}This approach:
- Eliminates duplicate disable/update checks
- Removes unnecessary inheritance
- Keeps all functionality through focused utility functions
- Improves maintainability by centralizing common logic
| ?multiple=${field.fieldOptions?.multiple} | ||
| ?required=${field.fieldOptions?.required} | ||
| @change=${async (e: Event) => { | ||
| if (!field.fileOptions?.tusOptions?.endpoint) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!field.fileOptions?.tusOptions?.endpoint) return; | |
| if (!field.fileOptions?.tusOptions?.endpoint) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| if (inputField.type !== "file") { | ||
| throw new Error("inputField is not a valid file element"); | ||
| public async upload(dataText = "test value") { | ||
| if (this.el.getAttribute("disable")) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (this.el.getAttribute("disable")) return; | |
| if (this.el.getAttribute("disable")) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| switchField.click(); | ||
| await this.component.updateComplete; | ||
| public async toggle() { | ||
| if (this.el.getAttribute("disable")) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (this.el.getAttribute("disable")) return; | |
| if (this.el.getAttribute("disable")) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| numberOfClicks = 1 | ||
| ) => { | ||
| public select = async (label: number | string) => { | ||
| if (this.el.getAttribute("disable")) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (this.el.getAttribute("disable")) return; | |
| if (this.el.getAttribute("disable")) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| public click = async (numberOfClicks = 1) => { | ||
| if (this.el.getAttribute("disable")) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (this.el.getAttribute("disable")) return; | |
| if (this.el.getAttribute("disable")) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| export const elementIsVisible = (element: HTMLElement) => { | ||
| if (!element) return false; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!element) return false; | |
| if (!element) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
hey @anuragxxd, have you had a chance to take a look at this? |
Description
This PR aims to simplify and expand tests for the ecc-utils design form, it also sets a standard for how the other tests for how tests for the other components will be written.
Summary by Sourcery
Expand and refactor tests for the ecc-utils design form to cover a wider range of input types and field options, and improve test component modularity.
Enhancements:
Tests:
Chores: