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

Support source maps #1862

Merged
merged 1 commit into from
Nov 9, 2020
Merged

Support source maps #1862

merged 1 commit into from
Nov 9, 2020

Conversation

JannesMeyer
Copy link
Contributor

@JannesMeyer JannesMeyer commented Nov 2, 2020 •
edited

Description

Node.js >=12.12.0 has support for source maps when run with the flag --enable-source-maps but the additional stack trace information is not being shown by Jasmine's stack traces. This pull request stops stack trace information from being discarded. Among other things, this causes the source map information to show up.

Motivation and Context

Currently Jasmine discards source map information from stack traces when the file property is not set on those stack frames.
This would proably fix #491.

It might be relevant that the "competing" framework Mocha has specific support for --enable-source-maps: https://mochajs.org/#-enable-source-maps

Personally, I would prefer a completely unmodified stack trace from Node/V8, because I think that most users are educated enough to be able to read those. The extra information is just too valuable.

How Has This Been Tested?

I have manually tested this with a test suite that throws an error. Before running Jasmine I had to set the following environment variable:

# Setting environment variables might look different in your shell. This is what I ran in PowerShell:
$env:NODE_OPTIONS = '--enable-source-maps'

The stack trace looks much more complete now and it finally includes the source map information. I am not sure if it's feasible to add an automated test, because the feature requires at least Node.js v12.12.0, so the test would fail on Node.js 10. Additionally, we would have to --enable-source-maps for the test process.

Output before:

  Stack:
        at <Jasmine>
        at UserContext.<anonymous> (C:\test\spec\spec.js:3:15)
        at <Jasmine>
        at processImmediate (node:internal/timers:462:21)

Output after:

  Stack:
    C:\test\spec\spec.ts:9
            throw new Error('This is a test');
                  ^
    Error: This is a test
        at UserContext.<anonymous> (C:\test\spec\spec.js:3:15)
            -> C:\test\spec\spec.ts:9:15
        at <Jasmine>
        at processImmediate (node:internal/timers:462:21)

Note that this includes the correct TypeScript filename as well as correct line/column numbers. Many editors (such as VS Code) can turn this into a clickable link that directly opens the source file.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@JannesMeyer JannesMeyer mentioned this pull request Nov 2, 2020
Open
@JannesMeyer
Copy link
Contributor Author

JannesMeyer commented Nov 2, 2020 •
edited

I am not sure why the CI test on Node v14 failed. Everything passed on v10 and v12 (and locally on v14 as well).

1) Env integration specDone reporting reports the duration of the spec

  Message:

    Expected 9 to be greater than or equal 10.

Seems like a rounding error to me? Is there any way to re-run the CI?

@sgravrock
Copy link
Member

Can you tell me how to reproduce the problem that this solves? I regularly use Jasmine in Node to test TypeScript code and I haven't seen a non-source-mapped stack trace in literally years, so I think there's probably an important difference between your setup and mine.

@JannesMeyer
Copy link
Contributor Author

JannesMeyer commented Nov 2, 2020 •
edited

Hmm, that's interesting. I just use the normal TypeScript compiler. No other tools involved (no webpack, etc.)

I have created a minimal example that shows the problematic behaviour: minimal-example.zip

Just run these three commands in the folder:

npm install
npm start
npm test

You should see output that contains the non-sourcemapped stack trace:

Randomized with seed 89343
Started
F

Failures:
1) test suite fails
  Message:
    Error: This is a test
  Stack:
    Error: This is a test
        at UserContext.<anonymous> (C:\minimal-example\spec\spec.js:5:15)
        at <Jasmine>
        at processImmediate (node:internal/timers:462:21)

1 spec, 1 failure
Finished in 0.005 seconds
Randomized with seed 89343 (jasmine --random=true --seed=89343)

I am curious about your setup as well. It would be great if there was a way to get it working without modifying Jasmine (as long as I don't need any extra npm dependencies... Trying to keep it minimal).

Node.js version: 15.0.1
Jasmine version: 3.6.3

@sgravrock
Copy link
Member

Thanks, that helps. I'm able to reproduce what you're seeing. I usually use @babel/register, which rewrites the existing stack trace lines with correct filenames and line numbers rather than adding an extra line to each frame.

The approach you've taken here looks fine to me. If you'll add a test for this (probably next to, and in the same style as, the existing stack trace filtering tests) and modify the source files in src/ rather than the build output in lib/, I'll merge it.

@sgravrock
Copy link
Member

The failure you saw on Node 14 is a flaky test that I haven't found the time to deal with. Don't worry about it.

@JannesMeyer
Copy link
Contributor Author

JannesMeyer commented Nov 4, 2020 •
edited

Thanks, that sounds great! I have followed your advice. Could you please review again?

In the test I used Windows-style paths with backslashes and starting with C:\ instead of http://. I thought that adding a little bit of variety to the tests couldn't hurt. These are the kinds of stack traces that I see in Node.js. Hopefully that is okay with you. Just let me know if I should change it to the same http:// paths as the other tests.

The rest should be in the same style as the existing tests.

@JannesMeyer
Copy link
Contributor Author

JannesMeyer commented Nov 4, 2020 •
edited

Sorry, I originally forgot to revert the changes to jasmine.js as your CONTRIBUTING.md suggests.

@JannesMeyer
Copy link
Contributor Author

I just squashed my commits into a single commit to make the history a bit cleaner

@sgravrock sgravrock merged commit 88de272 into jasmine:main Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: Sourcemap support
2 participants
@JannesMeyer @sgravrock