Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tests in TypeScript (.ts files) lack stack traces #8662

Closed
turadg opened this issue Dec 14, 2023 · 9 comments · Fixed by #9711
Closed

tests in TypeScript (.ts files) lack stack traces #8662

turadg opened this issue Dec 14, 2023 · 9 comments · Fixed by #9711
Assignees
@erights
Labels
devex developer experience enhancement New feature or request test tooling repo-wide infrastructure

Comments

@turadg
Copy link
Member

turadg commented Dec 14, 2023 •
edited
Loading

What is the Problem Being Solved?

The boot tests run .ts files in Ava using --loader=tsx. This works but due to the Endo hardening the stack traces lose line numbers. (Transpiled files are treat as being all in one line)

To observe

cd packages/boot
# edit a test file to induce an exception
yarn test yarn test test/bootstrapTests/test-addAssets.ts

See the .ts files in the stack trace all report line 1. E.g.,

  › Node.js v20.11.0
  › runPackageScript (packages/boot/tools/supports.ts:1:2676)
  › async Object.buildAndExtract [as buildProposal] (packages/boot/tools/supports.ts:1:3672)
  › async packages/boot/test/bootstrapTests/test-addAssets.ts:1:435

Description of the Design

endojs/endo#1798 was one hypothesis but may not solve the above.

Security Considerations

n/a

Scaling Considerations

n/a

Test Plan

Verify manually that stack traces have full line numbers.

Upgrade Considerations

n/a
@turadg turadg added enhancement New feature or request tooling repo-wide infrastructure test devex developer experience labels Dec 14, 2023
@turadg turadg changed the title tests with .ts files lack stack traces tests in TypeScript (.ts files) lack stack traces Feb 13, 2024
@dckc
Copy link
Member

dckc commented Feb 13, 2024

This is impacting me today. Not a big deal, but...

  use IBC callbacks after upgrade
  Value is not truthy:

  undefined

  › packages/boot/test/bootstrapTests/test-vats-restart.ts:1:4790

This was referenced Feb 28, 2024
Closed
Merged
@erights
Copy link
Member

erights commented Mar 1, 2024

I don't understand what's causing this, so I don't know if #2109 would help.

@turadg @dckc how can I reproduce?

@mhofman
Copy link
Member

mhofman commented Mar 2, 2024

I know what the problem is. Mark I can walk you through my potential fix this week.

@turadg
Copy link
Member Author

turadg commented Mar 4, 2024

@erights I've added repro steps to the body.

@turadg turadg mentioned this issue Jul 12, 2024
Closed
@erights erights mentioned this issue Jul 12, 2024
Closed
@erights
Copy link
Member

erights commented Jul 12, 2024

This is a bug in endo, not in agoric-sdk. Now filed as endojs/endo#2348 . Feel free to close out this one as subsumed by that one.

@erights
Copy link
Member

erights commented Jul 12, 2024

Thanks for the repro steps! I'll try to reproduce this.

@erights erights self-assigned this Jul 12, 2024
@erights
Copy link
Member

erights commented Jul 12, 2024

I assigned that one to myself. I am also assigning this to myself, and closing this one as subsumed by that one.

@erights erights closed this as completed Jul 12, 2024
@turadg
Copy link
Member Author

turadg commented Jul 12, 2024

I have this problem in agoric-sdk so I'll keep the ticket open until it's solved.

@turadg turadg reopened this Jul 12, 2024
@erights
Copy link
Member

erights commented Jul 12, 2024

Ok. Keep it assigned to me.

This was referenced Jul 14, 2024
Merged
Merged
erights added a commit to endojs/endo that referenced this issue Jul 16, 2024
@erights
fix(ses): work around #2348 linenumber bug (#2355)
00f5eab 
Closes: #2348 

Refs: Agoric/agoric-sdk#9711
#1798
#1799
Agoric/agoric-sdk#8662
Agoric/agoric-sdk#9700

## Description

Prior to this PR, when you ran on Ava on Node a test written in
TypeScript, you'd see something like the following in your stack traces.

```
  boot/test/bootstrapTests/stack-linenumbers.test.ts:1:104
```

This is because the TypeScript compiler compiles a TypeScripy file into
one line
of JavaScript with a sourceMap that should map back into original
source positions. Node specifically makes use of that sourceMap
to produce original line-numbers. However, Node does this in a way
that resists virtualization, so the normal SES error taming cannot use
this sourceMap info.

By default, this PR does not change this behavior. However it recognizes
a new `SUPPRESS_NODE_ERROR_TAMING` environment variable.

With the `SUPPRESS_NODE_ERROR_TAMING` environment variable absent
or set to `'disabled'`, you should still see stack traces as shown above

However, if you also set  the `SUPPRESS_NODE_ERROR_TAMING` environment
variable `'enabled'`, for example by doing

```sh
$ export SUPPRESS_NODE_ERROR_TAMING=enabled
```
at a bash shell, then when you run this test you should instead see
something like
```
boot/test/bootstrapTests/stack-linenumbers.test.ts:40:32
```

At Agoric/agoric-sdk#9711 I both 
- turn this PR into an agoric-sdk patch of endo, in order to emulate
this fix until the next endo-release-agoric-sdk-sync cycle, and
- add a test case that emits an error stack trace from an Ava test case
written in TypeScript, to test that it works.

### Security Considerations

This new behavior only applies when `errorTaming: 'unsafe'`, on v8, and
with this new environment variable enabled.

Setting `errorTaming: 'unsafe'` already flags to sacrifice some security
for a better debugging experience. But the loss of security is moderate
enough --- mostly confidentiality rather than integrity --- that some
may chose this setting for some production purposes.

The new behavior is a more severe loss of security that really should be
used ***only during development***, not production, when even a severe
loss of security is usually not an issue.

### Scaling Considerations

none
### Documentation Considerations

The behavior prior to this PR or without this environment variable
enabled is an unpleasant debugging experience. However, developers won't
know how to repair it, or even that it can be repaired, without
explanation. Even then, the difficultly of discovery in a problem.

The names `SUPPRESS_NODE_ERROR_TAMING` and the settings `'enabled'` and
`'disabled'` are by no means clear expressions of what this does.
Reviewers, ***better names would be appreciated!***

### Testing Considerations

The point. As developers write and run tests written in TypeScript, they
need to iterate with problems revealed by the tests, for which they need
good line numbers, including into the test code.

When the environment variable is enabled, the new behavior broke some
SES tests written specifically to test the old behavior. This would not
happen under CI because the environment variable is not set by default,
and so may not have been noticed. But it was revealed in local testing.
To repair this, this PR also sets those tests up to set
`process.env.SUPPRESS_NODE_ERROR_TAMING` to `'disabled'` before
lockdown, protecting those tests from the external environment variable
setting.

Awkwardly, at the moment Agoric/agoric-sdk#9711
serves as the only test of this PR. This is because I failed to figure
out how to configure things so I can run TypeScript tests under Ava,
like Agoric/agoric-sdk#9711 does. I tried cargo
culting the configs that seemed relevant, but it didn't work.

Reviewers, if you let me know how to do this, I'll duplicate the test
case from Agoric/agoric-sdk#9711 here, which
would be good.

### Compatibility Considerations

With the environment variable absent or disabled, there should be zero
difference in behavior, so none.

In a development environment where this environment variable is enabled,
some stack traces will be different. But outside of SES itself, nothing
should depend on the contents of stack traces, so again none.

### Upgrade Considerations

No upgrade considerations.

Nothing BREAKING.

- [x] Update `NEWS.md` for user-facing changes.
@mergify mergify bot closed this as completed in #9711 Jul 19, 2024
@mergify mergify bot closed this as completed in ac88da5 Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@erights erights

Labels
devex developer experience enhancement New feature or request test tooling repo-wide infrastructure
Projects
None yet
Milestone
No milestone
4 participants
@turadg @mhofman @dckc @erights