Skip to content

Conversation

@iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Dec 4, 2023

@iuioiua iuioiua requested a review from lucacasonato December 4, 2023 00:15
@github-actions github-actions bot added the http label Dec 4, 2023
@iuioiua iuioiua marked this pull request as ready for review December 4, 2023 00:17
@iuioiua iuioiua requested a review from kt3k as a code owner December 4, 2023 00:17
Comment on lines +160 to +171
await Deno.writeTextFile(filePath, text);

const req = new Request("http://localhost/test%20file.txt");
const res = await serveDir(req, serveDirOptions);
const downloadedFile = await res.text();

assertEquals(res.status, 200);
assertEquals(
res.headers.get("content-type"),
"text/plain; charset=UTF-8",
);
assertEquals(downloadedFile, text);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you surround these part with try {} block and call Deno.remove(filePath) in finally {} block? That structure avoids the leak of created file even when the test failed in the middle of the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I instead added the file to .gitignore, along with the files from #3879. Let me know if you'd still like me to add in the try/catch blocks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that works too 👍

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iuioiua iuioiua merged commit ea64542 into main Dec 4, 2023
@iuioiua iuioiua deleted the http-servedir-space-filename branch December 4, 2023 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants