fix: apply source-maps to test errors #884

bajtos · 2018-01-22T14:39:52Z

When we got rid of ts-node in #602, we also unintentionally dropped
the code modifying error stack traces to point to original TypeScript
sources instead of the transpiled JavaScript output.

As a result, assertion failures contained stack traces pointing to code
that we are not familiar with, which made troubleshooing difficult.

This commits add source-map-support as a dependency of our projects
and modifies shared mocha.opts to register it at runtime.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

shimks

Couple of iffys that I wasn't sure of, but aside from that LGTM

shimks · 2018-01-22T14:58:36Z

packages/build/package.json

    "posttest": "npm run lint"
  },
  "devDependencies": {
-    "fs-extra": "^5.0.0"
+    "fs-extra": "^5.0.0",
+    "source-map-support": "^0.5.2"


Since source-map-support is TypeScript specific, there's no need for it in build right?

Well build is using the same shared mocha.opts that TypeScript projects are using.

If we don't want to have source-map-support as part of build's mocha opts, then we would have to create (and maintain) another copy, one without --require source-map-support.

Not a big deal, I don't really mind either way.

shimks · 2018-01-22T14:58:49Z

packages/cli/package.json

@@ -33,6 +33,7 @@
    "nsp": "^3.1.0",
    "rimraf": "^2.6.2",
    "sinon": "^4.1.2",
+    "source-map-support": "^0.5.2",


Same as above

And same reply as above applies here too 😄

kjdelisle

+1 to @shimks ' feedback, other than that, LGTM

bajtos · 2018-01-22T15:53:31Z

Thank you for your review. I reworked mocha.opts and created two files:

mocha.ts.opts for TypeScript codebases
mocha.js.opts for JavaScript-only projects

See 254cbc0

LGTY now? @kjdelisle @shimks

shimks · 2018-01-22T18:05:13Z

packages/build/mocha.js.opts

+--recursive
+--exit
+--reporter dot
+--require source-map-support/register


No need for this here right?

raymondfeng · 2018-01-22T21:34:53Z

@bajtos Good catch. Is it possible to only add source-map-support as a dev dependency at root level package.json for loopback-next?

raymondfeng · 2018-01-22T21:36:09Z

Do you think we need to fix https://github.com/strongloop/loopback-next/blob/master/packages/cli/generators/project/templates/test/mocha.opts too?

raymondfeng · 2018-01-22T21:55:42Z

Is it possible to only add source-map-support as a dev dependency at root level package.json for loopback-next?

I confirm it's good enough to only add source-map-support to top-level loopback/next/package.json.

raymondfeng

Please only add source-map-support as a dev dep of loopback-next/package.json.

raymondfeng · 2018-01-23T00:20:40Z

It seems that nyc somehow gets in the way. Here is what I see from npm test:

Unhandled error in GET /hello: 500 InternalServerError: Bad hello
    at TestController.hello (/Users/rfeng/Projects/loopback4/loopback-next/packages/rest/dist/test/integration/http-handler.integration.js:6:8854)
    at ControllerRoute.invokeHandler (/Users/rfeng/Projects/loopback4/loopback-next/packages/rest/src/router/routing-table.ts:47:50)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

Please note http-handler.integration.js:6:8854 is not mapped but routing-table.ts:47:50 is.

npm run mocha produces a better result:

Unhandled error in GET /hello: 500 InternalServerError: Bad hello
    at TestController.hello (/Users/rfeng/Projects/loopback4/loopback-next/packages/rest/test/integration/http-handler.integration.ts:471:17)
    at ControllerRoute.invokeHandler (/Users/rfeng/Projects/loopback4/loopback-next/packages/rest/src/router/routing-table.ts:318:46)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

I tried different config of nyc but none of them helped.

bajtos · 2018-01-23T08:23:17Z

Is it possible to only add source-map-support as a dev dependency at root level package.json for loopback-next?

I confirm it's good enough to only add source-map-support to top-level loopback/next/package.json.

Good point, I'll remove source-map-support from most dev-dependencies. However, we need to preserve it in example packages, because they must work outside of our monorepo too.

bajtos · 2018-01-23T09:24:24Z

Do you think we need to fix https://github.com/strongloop/loopback-next/blob/master/packages/cli/generators/project/templates/test/mocha.opts too?

Good catch, will fix.

bajtos · 2018-01-23T10:04:22Z

It seems that nyc somehow gets in the way. Here is what I see from npm test:
[cut]
Please note http-handler.integration.js:6:8854 is not mapped but routing-table.ts:47:50 is.

npm run mocha produces a better result.

I looked into this issue, I believe it's a known problem of nyc - see istanbuljs/nyc#619 and istanbuljs/nyc#512.

https://github.com/istanbuljs/nyc#accurate-stack-traces-using-source-maps

When produce-source-map is set to true, then the instrumented source files will include inline source maps for the instrumenter transform. When combined with source-map-support, stack traces for instrumented code will reflect their original lines.

IIUC, nyc is modifying our transpiled .js files to add instrumentation code, and producing another source map allowing reporters to associate coverage data with the original .js files. I am guessing that this code producing source maps does not take into account the source map created by TypeScript? IDK.

AFAICT, the current master branch has the same problem, it's not something introduced by my changes here. I am proposing to leave this issue out of scope of this pull request.

bajtos · 2018-01-23T10:05:17Z

@raymondfeng I addressed your comments in 65cf212, PTAL.

When we got rid of ts-node in #602, we also unintentionally dropped the code modifying error stack traces to point to original TypeScript sources instead of the transpiled JavaScript output. As a result, assertion failures contained stack traces pointing to code that we are not familiar with, which made troubleshooing difficult. This commits add source-map-support as a global dev-dependency plus a dev-dependency of example repositories that must work outside of our monorepo too. The single shared `mocha.opts` was split into two different options files: - `mocha.ts.opts` which registers source-map support - `mocha.js.opts` with no support for source maps

This is a follow-up to #884. The changes make it impossible to run 'npm test' for individual packages. - There is no local mocha command for each package and it depends on the global installation of 'mocha' - With the global mocha, --require source-map-support/register cannot be resolved. The PR adds 'lb-mocha' command so that we can support source-map-support correctly along with other options. No global mocha installation is needed.

bajtos added this to the Sprint 53 milestone Jan 22, 2018

bajtos self-assigned this Jan 22, 2018

bajtos requested review from kjdelisle, raymondfeng and shimks as code owners January 22, 2018 14:39

bajtos added the needs-priority label Jan 22, 2018

shimks approved these changes Jan 22, 2018

View reviewed changes

kjdelisle approved these changes Jan 22, 2018

View reviewed changes

b-admike approved these changes Jan 22, 2018

View reviewed changes

shimks reviewed Jan 22, 2018

View reviewed changes

raymondfeng requested changes Jan 22, 2018

View reviewed changes

bajtos force-pushed the test/add-source-map-support branch from 254cbc0 to 65cf212 Compare January 23, 2018 10:04

bajtos mentioned this pull request Jan 23, 2018

fix: correctly report error numbers in stack traces istanbuljs/nyc#619

Open

raymondfeng approved these changes Jan 23, 2018

View reviewed changes

kjdelisle modified the milestones: Sprint 53, Sprint 54 Jan 24, 2018

bajtos force-pushed the test/add-source-map-support branch from 9994cf6 to 9da5f48 Compare January 25, 2018 08:43

bajtos merged commit 76a7f56 into master Jan 25, 2018

bajtos deleted the test/add-source-map-support branch January 25, 2018 09:17

bajtos removed the needs-priority label Jan 25, 2018

raymondfeng mentioned this pull request Jan 25, 2018

fix: make mocha self-contained with the source map support #913

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: apply source-maps to test errors #884

fix: apply source-maps to test errors #884

bajtos commented Jan 22, 2018 •

edited

Loading

shimks left a comment

shimks Jan 22, 2018

bajtos Jan 22, 2018

shimks Jan 22, 2018

bajtos Jan 22, 2018

kjdelisle left a comment

bajtos commented Jan 22, 2018

shimks Jan 22, 2018

raymondfeng commented Jan 22, 2018

raymondfeng commented Jan 22, 2018

raymondfeng commented Jan 22, 2018

raymondfeng left a comment

raymondfeng commented Jan 23, 2018

bajtos commented Jan 23, 2018

bajtos commented Jan 23, 2018

bajtos commented Jan 23, 2018

bajtos commented Jan 23, 2018

fix: apply source-maps to test errors #884

fix: apply source-maps to test errors #884

Conversation

bajtos commented Jan 22, 2018 • edited Loading

Checklist

shimks left a comment

Choose a reason for hiding this comment

shimks Jan 22, 2018

Choose a reason for hiding this comment

bajtos Jan 22, 2018

Choose a reason for hiding this comment

shimks Jan 22, 2018

Choose a reason for hiding this comment

bajtos Jan 22, 2018

Choose a reason for hiding this comment

kjdelisle left a comment

Choose a reason for hiding this comment

bajtos commented Jan 22, 2018

shimks Jan 22, 2018

Choose a reason for hiding this comment

raymondfeng commented Jan 22, 2018

raymondfeng commented Jan 22, 2018

raymondfeng commented Jan 22, 2018

raymondfeng left a comment

Choose a reason for hiding this comment

raymondfeng commented Jan 23, 2018

bajtos commented Jan 23, 2018

bajtos commented Jan 23, 2018

bajtos commented Jan 23, 2018

bajtos commented Jan 23, 2018

bajtos commented Jan 22, 2018 •

edited

Loading