chore: Add tests and improve code coverage.#28
chore: Add tests and improve code coverage.#28aakashH242 wants to merge 9 commits intohustcc:mainfrom
Conversation
chore: Add tests and improve coverage.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
@hustcc - Requesting your review. |
nice pr, thanks, I have send the code review. |
Fix PNG and SVG URLs not showing image.
Chore/add unit tests review
|
@hustcc - thanks for your patience. I have updated the PR and addressed your comments. Please note that after switching to running actual tools calls instead of mocks during tests, some of the tests randomly timeout in the CI pipeline in Github. I am not sure how to fix it so I have disabled those tests in the GitHub CI pipeline. They will still run on developer's local and be enforced by the pre-commit hook. I hope you find this implementation satisfactory. Please do review at your convenience. Thanks! |
|
@hustcc - did you get a chance to review the PR? |
|
@aakashH242 Hi, there seems to be too much content in diff, please try to minimize unnecessary diff~ |
| npm run build | ||
| npx vitest --watch=false | ||
| echo "Checking coverage threshold..." | ||
| npm run test:coverage 2>&1 | grep -E "All files.*100.*100.*100.*100" || echo "Coverage check completed" |
There was a problem hiding this comment.
no need to check coverage threshold handly.
| @@ -0,0 +1,81 @@ | |||
| # Quick CI Reference | |||
There was a problem hiding this comment.
Remove all the markdown file, just add some guide in readme.md
| #!/bin/sh | ||
| echo "Running lint-staged" | ||
| npx lint-staged No newline at end of file | ||
|
|
| "type": "string", | ||
| "minLength": 1, | ||
| "description": "The mermaid diagram syntax used to be generated, such as, graph TD;\nA-->B;\nA-->C;\nB-->D;\nC-->D;." | ||
| "description": "The mermaid diagram syntax to be generated. Use 'flowchart' instead of 'graph' for v10+ compatibility. Example:\nflowchart TD\n A-->B\n A-->C\n B-->D\n C-->D\n\nOther diagram types: sequenceDiagram, classDiagram, stateDiagram, erDiagram, gantt, pie, etc." |
There was a problem hiding this comment.
We can give a case for each graph, and use multi line promts.
| @@ -0,0 +1,48 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
| import { schema, tool } from "../../src/tools"; | |||
| describe("tools", () => { | |||
| B-->D | ||
| C-->D | ||
|
|
||
| Other diagram types: sequenceDiagram, classDiagram, stateDiagram, erDiagram, gantt, pie, etc.`) |
| export { renderMermaid } from "./render"; | ||
| export { Logger } from "./logger"; | ||
| export { createMermaidInkUrl } from "./mermaidUrl"; | ||
|
|
| mermaidConfig.themeVariables = { | ||
| background: options.backgroundColor, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Why is there this configuration processing logic?
|
@aakashH242 I found that there is too much content in this PR and I am unable to review it correctly! |
Hi @hustcc , thanks for your new comments. I apologize for the HUGE PR as it was necessary to have such a huge diff to address all testing scenarios and get a 100% code coverage. May I suggest using AI tools to help you with your review in case the diff is too big to manually skim through it? |
|
@aakashH242 Several principles:
|
|
We need to do a few things:
And remove all other diffs. |
|
@aakashH242 So we should do things base this pr:
|
3 participants
Tests added and code coverage improved to 100%.
Add contributing and testing guidelines.
Setup CI workflows.