Skip to content

fix: error stack traces in transpiled TypeScript #740

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

Merged
merged 6 commits into from
Apr 23, 2025

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Apr 17, 2025

This is a follow-up for #717, where we added TypeScript transpilation but did not test how it works in practice.

The implementation is based on this Deno example:
https://github.com/denoland/deno_core/blob/0.343.0/core/examples/ts_module_loader.rs

Example code:

interface User {
  name: string;
  email: string;
}

const err: Error = new Error("boom!"); throw err;

Deno output:

error: Uncaught (in promise) Error: boom!
const err: Error = new Error("boom!"); throw err;
                   ^
    at file:///project/x.ts:6:20

Zinnia before this change - notice the missing : Error snippet and wrong source code line & column numbers:

error: Uncaught (in promise) Error: boom!
const err = new Error("boom!");
            ^
    at file:///project/x.ts:1:13

Zinnia after this change (same output as from Deno):

error: Uncaught (in promise) Error: boom!
const err: Error = new Error("boom!"); throw err;
                   ^
    at file:///project/x.ts:6:20

Links:

Related:

Example code:

```ts
interface User {
  name: string;
  email: string;
}

const err: Error = new Error("boom!"); throw err;
```

Deno output:

```
error: Uncaught (in promise) Error: boom!
const err: Error = new Error("boom!"); throw err;
                   ^
    at file:///project/x.ts:6:20

Zinnia before this change - notice the missing `: Error` text
and wrong source code line & column numbers:

```
error: Uncaught (in promise) Error: boom!
const err = new Error("boom!");
            ^
    at file:///project/x.ts:1:13
```

Zinnia after this change (same output as from Deno):

```
error: Uncaught (in promise) Error: boom!
const err: Error = new Error("boom!"); throw err;
                   ^
    at file:///project/x.ts:6:20
```

Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos requested a review from pyropy as a code owner April 17, 2025 12:45
@bajtos bajtos requested a review from NikolasHaimerl April 17, 2025 12:45
@bajtos bajtos requested a review from NikolasHaimerl as a code owner April 17, 2025 12:45
}
Ok(())
}

// Run all tests in a single JS file
async fn run_js_test_file(name: &str) -> Result<(Vec<String>, Option<AnyError>), AnyError> {
Copy link
Member Author

Choose a reason for hiding this comment

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

In retrospect, it was a mistake to use Option<AnyError> here. I would like to rework this to return Result<(), AnyError> instead, but such refactoring is out of scope of this pull request.

@NikolasHaimerl, do you have a suggestion on how to best represent a type with the following variants in Rust?

  • We cannot execute the JS/TS file -> Err(AnyError)
  • We executed the JS/TS file and collected some logs in Vec<String>. In addition to these logs, we want to provide:
    • The script executed cleanly -> ()
    • The script threw a runtime error -> AnyError

Variants:

  1. Err(AnyError)
  2. Ok((Ok(()), Vec<String>))
  3. Ok((Err(AnyError), Vec<String>))

Choose a reason for hiding this comment

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

How about we wrap the logs inside the Anyhow context string?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you mean. We need to extract the logs from the results, so that we can assert what exactly was logged by the test runner on failure. Do you know how to extract one piece of a context from the Anyhow error?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am leaning towards using Result<(Result<(), AnyError>, Vec<String>), AnyError>.
I.e. Result<ScriptResult, AnyError> where ScriptResult is a tuple (Result<(), AnyError>, Vec<String>).

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it help to create a named struct ScriptResult?

Alternatively, maybe we can treat the case "we cannot execute the script" the same way as "the script threw an error".

That will give us (Result<(), AnyError>, Vec<String>)

  • We cannot execute the JS/TS file -> (Err(AnyError), vec![])
  • The script executed cleanly -> (Ok(()), logs)
  • The script threw an error -> (Err(AnyError), logs)

WDYT?

I'd also like to make this refactoring as a follow-up, after I land the remaining two pull requests.

Choose a reason for hiding this comment

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

I see what you mean. Ideally I would like to avoid having a result inside a result. We could use enums as error types to give us specific information about which error was caused by the test runner. Why do we not unwrap an error once it occurs? What is the benefit of propagating it upwards if it is only used in testing?
Usually, in Rust testing, I would expect Errors to be unwrapped as they occur within the testing suite. Error propagation, I would usually only do in production.

Copy link
Member Author

@bajtos bajtos Apr 24, 2025

Choose a reason for hiding this comment

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

Makes sense.

Why do we not unwrap an error once it occurs?

In some tests, we want to assert that execution of the JS code threw an error and that the JS code produced the expected logs.

What is the benefit of propagating it upwards if it is only used in testing?
Usually, in Rust testing, I would expect Errors to be unwrapped as they occur within the testing suite. Error propagation, I would usually only do in production.

I find it easier when all test helpers use error propagation instead of unwrapping, because that way I don't have to decide in each test helper which error-handling strategy is appropriate.

Let's continue this discussion in #752

Comment on lines 106 to 113
let actual_error = format_js_error(e);
// Strip ANSI codes (colors, styles)
let actual_error = console_static_text::ansi::strip_ansi_codes(&actual_error);
// Replace current working directory in stack trace file paths with a fixed placeholder
let cwd_url = ModuleSpecifier::from_file_path(std::env::current_dir().unwrap()).unwrap();
let actual_error = actual_error.replace(cwd_url.as_str(), "file:///project-root");
// Normalize line endings to Unix style (LF only)
let actual_error = actual_error.replace("\r\n", "\n")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is similar to the code we have in format_test_activities() below, but not exactly the same. WDYT, is it worth extracting a shared helper function?

fn format_test_activities(events: &[String]) -> String {
// Find all durations (e.g. `0ms` or `241ms`)
let duration_regex = regex::Regex::new(r"\d+ms").unwrap();
// Find trailing whitespace on all lines. Note that a chunk can be multi-line!
let eol_regex = regex::Regex::new(r" *\r?\n").unwrap();
let cwd_url = ModuleSpecifier::from_file_path(std::env::current_dir().unwrap()).unwrap();
events
.iter()
.map(|chunk| {
// Strip ANSI codes (colors, styles)
let chunk = console_static_text::ansi::strip_ansi_codes(chunk);
// Remove `console.info: ` added by RecordingReporter.
// Don't remove other `console` level prefixes, so that we can detect them.
let chunk = match chunk.strip_prefix("console.info: ") {
Some(remainder) => remainder,
None => &chunk,
};
// Replace current working directory in stack trace file paths with a fixed placeholder
let chunk = chunk.replace(cwd_url.as_str(), "file:///project-root");
// Normalize durations
let chunk = duration_regex.replace_all(&chunk, "XXms");
// Remove trailing whitespace before all EOLs
let chunk = eol_regex.replace_all(&chunk, "\n");
// Format the line, adding back EOL after trimming whitespace at the end
format!("{}\n", chunk.trim_end())
})
.collect::<Vec<String>>()
.join("")
}

Signed-off-by: Miroslav Bajtoš <[email protected]>
@@ -21,7 +23,9 @@ use deno_core::anyhow::Result;
pub struct ZinniaModuleLoader {
module_root: Option<PathBuf>,
// Cache mapping file_name to source_code
code_cache: Arc<RwLock<HashMap<String, String>>>,
code_cache: Rc<RefCell<HashMap<String, String>>>,

Choose a reason for hiding this comment

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

Is the module loader exclusively single threaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question! I am not sure, TBH.

We are running the Deno runtime on the current thread, see here:

#[tokio::main(flavor = "current_thread")]

#[tokio::main(flavor = "current_thread")]

I think that means all async tasks will be executed on the current thread too, including the asynchronous work of the module loader.

Deno uses Rc<RefCell<...>> in their example:

https://github.com/denoland/deno_core/blob/0.343.0/core/examples/ts_module_loader.rs

My knowledge of async Rust is very limited, I don't understand this deeply enough. What would you suggest to use?

Choose a reason for hiding this comment

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

Ah I see, well Rc<RefCell<...>> is used in a single-threaded environment, since Deno is using that as an example, I would assume it is single threaded. If we were to use multithreaded environments, I'd rather go for a Arc<Mutex<...>> type.

Copy link

@NikolasHaimerl NikolasHaimerl left a comment

Choose a reason for hiding this comment

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

Approving, as the result structure can be improved in a follow up PR

@bajtos bajtos enabled auto-merge (squash) April 23, 2025 07:41
@bajtos bajtos merged commit d861951 into main Apr 23, 2025
17 checks passed
@bajtos bajtos deleted the test-typescript-modules branch April 23, 2025 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants