-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Reviewer's Guide by SourceryThis pull request introduces unit tests for Class diagram showing the refactored ComHandler moduleclassDiagram
class ComHandler {
-String knit_job_id
-Boolean knit_status
-Boolean isStarted
+refreshPorts()
+updateMachineType()
+configureKnit()
+createJob()
+startKnit()
}
note for ComHandler "Refactored from global functions to module pattern"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
TvisharajiK
commented
Feb 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @TvisharajiK - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider enhancing the auto-generated tests with more edge cases and error conditions. The current tests mainly cover happy paths but should also verify behavior with invalid inputs and error scenarios.
- The drawingtool.test.js tests are quite basic - consider adding tests for different drawing operations, boundary conditions, and canvas state verification.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Replace arbitrary setTimeout delay with proper async flow control
Using a fixed 1000ms delay could cause race conditions. Consider using Promises or async/await to properly sequence initKnitJob and configKnitJob calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Missing edge case: empty previewTable
The test currently assumes there are rows in the previewTable. Add a test case where the table is empty to ensure colour_count is handled correctly when no rows are present. This will help verify the robustness of the configureKnit function in different scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add test case for when knit_job_id is empty or null
It's important to test how the function behaves when knit_job_id is not set. This could reveal potential issues or unexpected behavior in the code when the ID is missing.
Suggested implementation:
expect(configKnitJobMock).toHaveBeenCalledWith('mockJobId', 'mockDataUrl', 2, 'embedded'); }); it('should handle null knit_job_id appropriately', () => { const dom = new JSDOM(` <button id="configure-btn"></button> <button id="start-btn"></button> <table id="previewTable"><tr></tr><tr></tr></table> <canvas id="canvas2"></canvas> `); global.document = dom.window.document; global.window = dom.window; const canvas = document.getElementById('canvas2'); canvas.toDataURL = jest.fn(() => 'mockDataUrl'); comHandler.knit_job_id = null; comHandler.configureKnit(); expect(document.getElementById('configure-btn').disabled).toBe(false); expect(document.getElementById('start-btn').disabled).toBe(true); expect(initKnitJobMock).not.toHaveBeenCalled(); expect(configKnitJobMock).not.toHaveBeenCalled(); }); it('should handle empty knit_job_id appropriately', () => { const dom = new JSDOM(` <button id="configure-btn"></button> <button id="start-btn"></button> <table id="previewTable"><tr></tr><tr></tr></table> <canvas id="canvas2"></canvas> `); global.document = dom.window.document; global.window = dom.window; const canvas = document.getElementById('canvas2'); canvas.toDataURL = jest.fn(() => 'mockDataUrl'); comHandler.knit_job_id = ''; comHandler.configureKnit(); expect(document.getElementById('configure-btn').disabled).toBe(false); expect(document.getElementById('start-btn').disabled).toBe(true); expect(initKnitJobMock).not.toHaveBeenCalled(); expect(configKnitJobMock).not.toHaveBeenCalled(); });
Note: These test cases assume that when knit_job_id is null or empty:
- The configure button should remain enabled
- The start button should be disabled
- Neither initKnitJob nor configKnitJob should be called
You may need to adjust the expected behaviors based on how your application actually handles these cases. If the actual behavior is different, modify the expectations in the test cases accordingly.
@febuz
febuz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests. I reviewed and tried running them locally; the Jest suite passes (8/8).
However, the refactor of "app/controller/com_handler.js" from standalone functions to a CommonJS module breaks the existing UI. The HTML buttons in app/view/index.html still call global functions:
- onclick="refreshPorts()"
- onclick="updateMachineType()"
- onclick="configureKnit()"
- onclick="startKnit()"
After this PR those functions are no longer in the global scope, so the buttons will throw ReferenceError in the browser.
To resolve this while keeping the tests working, add global window bindings at the bottom of com_handler.js:
I applied this fix locally and tests still pass. Please update the PR and I'll be happy to approve.
@febuz
febuz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests. I reviewed and tried running them locally; the Jest suite passes (8/8).
However, the refactor of app/controller/com_handler.js from standalone functions to a CommonJS module breaks the existing UI. The HTML buttons in app/view/index.html still call global functions:
onclick="refreshPorts()"onclick="updateMachineType()"onclick="configureKnit()"onclick="startKnit()"
After this PR those functions are no longer in the global scope, so the buttons will throw ReferenceError in the browser.
To resolve this while keeping the tests working, add global window bindings at the bottom of com_handler.js:
module.exports = comHandler; if (typeof window !== 'undefined') { window.refreshPorts = comHandler.refreshPorts; window.updateMachineType = comHandler.updateMachineType; window.configureKnit = comHandler.configureKnit.bind(comHandler); window.createJob = comHandler.createJob.bind(comHandler); window.startKnit = comHandler.startKnit.bind(comHandler); }
I applied this fix locally and tests still pass. Please update the PR and I'll be happy to approve.
febuz
commented
Jun 17, 2026
Hi @TvisharajiK — I reviewed this and it needs one fix before merge: the refactor to a CommonJS module breaks the global onclick handlers in app/view/index.html. I opened #31 with the fix applied on top of your branch so it can be merged cleanly. If you prefer, you can apply the window-binding change from #31 to this PR and I'll close #31.
Uh oh!
There was an error while loading. Please reload this page.
This pull request introduces comprehensive unit tests for the com_handler.js and drawingtool.js module, significantly increasing the coverage of the file.
The new tests cover various scenarios, ensuring the reliability and robustness of the functionalities.
Came across Knitweb while exploring Js based projects that had a good star count.
Let me know if these test cases have added any value
Screenshot 2025年02月05日 at 2 59 40 PMSummary by Sourcery
Add unit tests for the
com_handler.jsanddrawingtool.jsmodules.Tests:
com_handler.jsmodule, covering functions likerefreshPorts,updateMachineType,configureKnit,createJob, andstartKnit.drawingtool.jsmodule, focusing on mouse events and layout updates.