fix(cli/test): decoding percent-encoding(non-ASCII) file path correctly #23200

Hajime-san · 2024-04-03T09:54:36Z

Summary

This PR resolves about the issue.
fixes #10810

And the formerly context is in the PR.
#22582

Here is an expected behaviour example with this change.

🦕.test.ts

import { assertEquals } from "https://deno.land/[email protected]/assert/mod.ts";

Deno.test("example test", () => {
  assertEquals("🍋", "🦕");
});

terminal

% /path/to/deno test <0001f995>.test.ts
Check file:///path/to/repo/🦕.test.ts
running 1 test from ./🦕.test.ts
example test ... FAILED (3ms)

 ERRORS 

example test => ./🦕.test.ts:3:6
error: AssertionError: Values are not equal.


    [Diff] Actual / Expected


-   🍋
+   🦕


  throw new AssertionError(message);
        ^
    at assertEquals (https://deno.land/[email protected]/assert/assert_equals.ts:52:9)
    at file:///path/to/repo/%F0%9F%A6%95.test.ts:4:3

 FAILURES 

example test => ./🦕.test.ts:3:6

FAILED | 0 passed | 1 failed (29ms)

error: Test failed

related issues

There are test reporter and runtime problems.
#18983

How we handle runtime behaviour?

The terminal output log not only contains the test reporter but also runtime file path treatment as follows.
at file:///path/to/repo/%F0%9F%A6%95.test.ts:4:3

spec

It seems that there is no specification for Error.stack today, and it is implementation-defined behaviour by browser engines.
(Thanks for the article, @petamoriken)

https://github.com/tc39/proposal-error-stacks

behaviour

Here is an example code to show stack trace, and runs it on modern browsers.

index.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <script type="module" src="./🍋.js"></script>
</head>
<body>
</body>
</html>

🍋.js

throw new Error('🍋')

result

Google Chrome(122.0.6261.112)

🍋.js:2 Uncaught Error: 🍋
    at 🍋.js:2:7
(anonymous) @ 🍋.js:2

I think that the stack trace implementation is located as follows.

https://source.chromium.org/chromium/chromium/src/+/refs/tags/122.0.6261.112:v8/src/inspector/v8-stack-trace-impl.cc;l=351-367

Firefox(117.0)

Uncaught Error: 🍋
    <anonymous> http://localhost:8080/🍋.js:1

Safari(17.3.1)

Error: 🍋
module code - 🍋.js:2

non-browser result

Node.js(20.12.0)

% node 🍋.mjs         
file:///path/to/%F0%9F%8D%8B.mjs:1
throw new Error('🍋')
      ^

Error: 🍋
    at file:///path/to/%F0%9F%8D%8B.mjs:1:7
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12)

deno implementation context

deno/runtime/fmt_errors.rs

Line 37 in 92a8ada

result += &cyan(&format_file_name(&file_name)).to_string();

https://github.com/denoland/deno_core/blob/5c3d303a6de112bb704620d8ba5956acf4e10a48/core/error.rs#L658

bartlomieju · 2024-04-14T22:25:46Z

Thanks for the PR @Hajime-san! It looks good, but it would be really great if you could exercise this change in an integration test. Would you be able to add a test to tests/integrations/test_tests.rs using the itest! macro?

Hajime-san · 2024-04-16T02:07:33Z

Hi @bartlomieju, thank you for pointing out about testing. I added it!
And, I found the lack of percent-encoding(non-ASCII) file path for checking types in running test when I was adding the integration test, so I commit it.

bartlomieju

Thanks!

Hajime-san · 2024-05-23T02:53:51Z

Thank you! @bartlomieju

BTW, how do you think to treat runtime behaviour that I wrote to the description of this PR?
I'm not familiar with the behavior of the v8 engine, but it may affect runtime performance.
If you and the core team have something to think about, I'd like to move this into an issue.

bartlomieju · 2024-05-27T20:42:11Z

Thank you! @bartlomieju

BTW, how do you think to treat runtime behaviour that I wrote to the description of this PR? I'm not familiar with the behavior of the v8 engine, but it may affect runtime performance. If you and the core team have something to think about, I'd like to move this into an issue.

Sorry for a slow response. As a rule of thumb if something works in browsers and makes sense in Deno context we should follow browser behavior. So this is a good fix 👍

bartlomieju · 2024-05-27T20:46:33Z

@Hajime-san looks like GH actions doesn't like that we now have a file that contains an emoji. Any chance you could rewrite the test to create this file, run relevant subcommand and then delete the file (or use a temp dir)?

Hajime-san · 2024-05-28T05:17:52Z

@bartlomieju

Sorry for a slow response. As a rule of thumb if something works in browsers and makes sense in Deno context we should follow browser behavior. So this is a good fix 👍

Sure! I moved to deno_core denoland/deno_core#757

looks like GH actions doesn't like that we now have a file that contains an emoji. Any chance you could rewrite the test to create this file, run relevant subcommand and then delete the file (or use a temp dir)?

I fixed it! It seems to pass all CI.

fix(cli/test): decode percent-encoding file path

5465c96

Hajime-san added 2 commits April 16, 2024 10:53

fix(cli/test): decode percent-encoding file path when checking types

1694535

adding integration test

5bcf180

bartlomieju added this to the 1.43 milestone Apr 16, 2024

bartlomieju and others added 2 commits April 21, 2024 00:33

Merge branch 'main' into fix-test-reporter-path-encoding

2a0716f

Merge branch 'main' into fix-test-reporter-path-encoding

781090e

bartlomieju modified the milestones: 1.43, 1.44 May 5, 2024

bartlomieju added 2 commits May 23, 2024 01:53

Merge branch 'main' into fix-test-reporter-path-encoding

6d95413

allow one more itest

6dd9e07

bartlomieju approved these changes May 22, 2024

View reviewed changes

bartlomieju enabled auto-merge (squash) May 22, 2024 23:54

Merge branch 'main' into fix-test-reporter-path-encoding

5a0cb9b

integration test using temporary file

e6941d2

auto-merge was automatically disabled May 28, 2024 02:32
Head branch was pushed to by a user without write access

Hajime-san added 2 commits May 28, 2024 11:59

decrement itest count

6df4695

Merge branch 'main' into fix-test-reporter-path-encoding

f430f60

Hajime-san mentioned this pull request May 28, 2024

decoding percent-encoding source string in Error.stack to follow browser behaviour denoland/deno_core#757

Closed

Hajime-san and others added 2 commits May 28, 2024 21:35

fix

a2d5747

Merge branch 'main' into fix-test-reporter-path-encoding

2a6ee04

bartlomieju enabled auto-merge (squash) May 28, 2024 13:09

bartlomieju merged commit 9aa593c into denoland:main May 28, 2024
17 checks passed

Hajime-san deleted the fix-test-reporter-path-encoding branch May 28, 2024 14:38

Hajime-san mentioned this pull request Jul 25, 2024

fix: upgrade deno_core to 0.298.0 #24709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli/test): decoding percent-encoding(non-ASCII) file path correctly #23200

fix(cli/test): decoding percent-encoding(non-ASCII) file path correctly #23200

Hajime-san commented Apr 3, 2024 •

edited

Loading

bartlomieju commented Apr 14, 2024

Hajime-san commented Apr 16, 2024

bartlomieju left a comment

Hajime-san commented May 23, 2024

bartlomieju commented May 27, 2024

bartlomieju commented May 27, 2024 •

edited

Loading

Hajime-san commented May 28, 2024 •

edited

Loading

fix(cli/test): decoding percent-encoding(non-ASCII) file path correctly #23200

fix(cli/test): decoding percent-encoding(non-ASCII) file path correctly #23200

Conversation

Hajime-san commented Apr 3, 2024 • edited Loading

Summary

related issues

How we handle runtime behaviour?

spec

behaviour

result

Google Chrome(122.0.6261.112)

Firefox(117.0)

Safari(17.3.1)

non-browser result

Node.js(20.12.0)

deno implementation context

bartlomieju commented Apr 14, 2024

Hajime-san commented Apr 16, 2024

bartlomieju left a comment

Choose a reason for hiding this comment

Hajime-san commented May 23, 2024

bartlomieju commented May 27, 2024

bartlomieju commented May 27, 2024 • edited Loading

Hajime-san commented May 28, 2024 • edited Loading

Hajime-san commented Apr 3, 2024 •

edited

Loading

bartlomieju commented May 27, 2024 •

edited

Loading

Hajime-san commented May 28, 2024 •

edited

Loading