-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: add more test coverage #88
Conversation
@@ -2,7 +2,7 @@ | |||
"tasks": { | |||
"build": "deno run --allow-read --allow-write --allow-net --allow-run --allow-env ./style/patch.ts && deno fmt", | |||
"check:types": "deno check **/*.ts", | |||
"coverage": "deno test --allow-read --coverage=cov_profile", | |||
"coverage": "rm -rf cov_profile && deno test --allow-read --coverage=cov_profile", |
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.
This made testing things easier. I don't think this causes any problems either.
@@ -21,4 +21,4 @@ export { default as sanitizeHtml } from "https://esm.sh/[email protected]?tar | |||
|
|||
export { escape as htmlEscape } from "https://esm.sh/[email protected]?pin=v135"; | |||
|
|||
export { default as katex } from "https://esm.sh/[email protected]/dist/katex.mjs?pin=v135"; | |||
export { default as katex } from "https://esm.sh/[email protected]?pin=v135"; |
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.
I'm not sure why we had the .mjs
like this, since it prevents types and documentation from working. Any thoughts here?
image(src: string, title: string | null, alt: string) { | ||
return `<img src="${src}" alt="${alt}" title="${title ?? ""}" />`; |
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.
The "issue" with coverage that I previously asked about wasn't an issue -- we just weren't testing every branch!
In the course of trying to get 100% coverage, I found that it's impossible for alt
to be null. If you cmd+click on the function, you'll see it's typed like this:
image(href: string, title: string | null, text: string): string;
so this just corrects our implementation.
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.
Good catch!
@@ -246,7 +246,7 @@ export function render(markdown: string, opts: RenderOptions = {}): string { | |||
allowedTags, | |||
allowedAttributes: { | |||
...sanitizeHtml.defaults.allowedAttributes, | |||
img: ["src", "alt", "height", "width", "align"], | |||
img: ["src", "alt", "height", "width", "align", "title"], |
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.
Need to allow title here, since this is valid markdown:
![](image.jpg "best title")
], | ||
a: ["id", "aria-hidden", "href", "tabindex", "rel", "target"], | ||
a: ["id", "aria-hidden", "href", "tabindex", "rel", "target", "title"], |
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.
Need to allow title here, since this is valid markdown:
[link](https://example.com "asdf")
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.
Your points seems all reasonable to me. Thank you!
@hashrock, this brings the coverage to 100%.