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.

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

Fix #1651: add support for nodejs experimental-modules stacktraces #1662

Closed
wants to merge 3 commits into from
Closed

Fix #1651: add support for nodejs experimental-modules stacktraces #1662

wants to merge 3 commits into from

Conversation

jehon
Copy link

@jehon jehon commented Mar 8, 2019

Add support for nodejs --experimental-modules.

Description

When using node --experimental-modules, the stackstrace are not string anymore, they are an array of "CallSites".

See https://nodejs.org/api/esm.html

Motivation and Context

Jasmine was failing with something like [...] .split('\n') [...]

How Has This Been Tested?

This is difficult to test with the current setup, because you need to restart node with --node-modules.
I did test that by running tests on my own project.

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.
  • [no] I have added tests to cover my changes.
  • [yes] All new and existing tests passed.

@slackersoft
Copy link
Member

Please take a look at the CONTRIBUTING document. This pull request right now, doesn't include any tests for the new functionality, or any code changes in src, but only changes the generated jasmine.js file. As such any merge of this will be overwritten without any kind of alert by the next grunt buildDistribution. Please add the code to the appropriate source file and we will generate jasmine.js when the PR is merged.

Additionally, Jasmine has its own functions to detect "Arrayness" (j$.isArray_) that should be used for browser compatibility.

Thanks for taking a look at this. Hope this helps. Thanks for using Jasmine!

@jehon
Copy link
Author

jehon commented Mar 18, 2019

Waw, a lot more complex than expected...
How could I test this feature, that requires node to run with "--experimental-modules" startup flag?

@slackersoft
Copy link
Member

Jasmine's test suite is mostly unit tests right now, so a test that calls StackTrace with a thing that looks like the new stack traces is probably for now. I would also be happy to see another entry in the travis build matrix that runs a suite with the flag set, but that will probably be more fiddly to get correct.

@jehon
Copy link
Author

jehon commented Mar 19, 2019

Hello,

Thanks for your reply. I will look if we can build up a "Stacktrace that look like that" :-)

To test that in action:

node --experimental-modules ./node_modules/.bin/grunt jshint execSpecsInNode

For the travis build, I will look at it when I will have time... Could you perhaps help on that point?

@slackersoft
Copy link
Member

If we want to add an entry to the travis-ci matrix you should be able to add one with NODE_OPTIONS="--experimental-modules" and the appropriate node version. I'm not sure it will actually fail though, when I run NODE_OPTIONS="--experimental-modules" npm test on my machine with node v10.13.0 everything executes just fine (although the tests are green right now).

For the unit tests, remember that the thing being formatted doesn't have to actually be CallSite objects just provide enough functionality that Jasmine's StackTrace can interact with them like they are.

Hope this helps.

@jehon
Copy link
Author

jehon commented Jun 21, 2019

Hello

This work is too huge for me to take it now. If someone want to take over, please feel free to do so.

Jean

dotnetCarpenter added a commit to dotnetCarpenter/utils that referenced this pull request Jun 30, 2019
@sgravrock
Copy link
Member

@jehon Thanks for your contribution. Can you provide more information on how to reproduce the problem that this PR solves? I've been unable to coax a stack trace that isn't a string out of Node. Here's what I've done so far:

  1. Patched jasmine-npm to load specs using import().
  2. Wrote a spec that throws an exception, and put it in a spec file with a name ending in .mjs.
  3. Ran Jasmine under Node 10.16.3, 11.9.0, 12.18.3, 13.5.0 using --experimental-modules.

In all cases Jasmine is able to correctly format and filter the stack trace.

@jehon
Copy link
Author

jehon commented Sep 25, 2020

Thanks for all your trials.

It has been a long time, and I didn't see the problem coming back since then. I do remember that, in the time I made that PR, exception handling was still under work in nodejs part. Perhaps it has been solved from they part.

I propose that we close this PR. If the problem arise back, someone could find it back and use it if necessary?

@sgravrock
Copy link
Member

I do remember that, in the time I made that PR, exception handling was still under work in nodejs part. Perhaps it has been solved from they part.

That was my guess as well. I'll go ahead and close this. Thanks again.

@sgravrock sgravrock closed this Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants