Cucumber's own features fail when parent directory contains parentheses

[eoola]

Describe the bug
Whenever the parent directory has parentheses in its name, I noticed the cucumber features tests start to fail. I also noticed that in the tests that are run, the path for the parent directory drops the parentheses from the path name. For the above reason, the path can not be found for a specified directory.

22) Scenario:  # features/usage_json_formatter.feature:8
    ✔ Before # ../../Cucumber x/cucumber-js/src/support_code_library_builder/build_helpers.ts:13
    ✔ Given a file named "features/a.feature" with: # ../../Cucumber x/cucumber-js/src/support_code_library_builder/build_helpers.ts:13
        """
        Feature: a feature
          Scenario: a scenario
            Given step A
            And step A
            When step B
            Then step C
        """
    ✔ And a file named "features/step_definitions/steps.js" with: # ../../Cucumber x/cucumber-js/src/support_code_library_builder/build_helpers.ts:13
        """
        const {When} = require('@cucumber/cucumber')

        When('step A', function() {});
        When('step B', function() {});
        When('step C', function() {});
        When(/step D/, function() {});
        """
    ✔ When I run cucumber-js with `--format usage-json` # ../../Cucumber x/cucumber-js/src/support_code_library_builder/build_helpers.ts:13
    ✖ Then it outputs the usage data: # ../../Cucumber x/cucumber-js/src/support_code_library_builder/build_helpers.ts:13
        | PATTERN | PATTERN_TYPE       | URI                                | LINE | NUMBER OF MATCHES |
        | step A  | CucumberExpression | features/step_definitions/steps.js | 3    | 2                 |
        | step B  | CucumberExpression | features/step_definitions/steps.js | 4    | 1                 |
        | step C  | CucumberExpression | features/step_definitions/steps.js | 5    | 1                 |
        | step D  | RegularExpression  | features/step_definitions/steps.js | 6    | 0                 |
        AssertionError
            + expected - actual

            -13
            +3

            at /Users/emmanuelola/Documents/Cucumber (x)/cucumber-js/features/step_definitions/usage_json_steps.ts:15:30
            at Array.forEach (<anonymous>)
            at World.<anonymous> (/Users/emmanuelola/Documents/Cucumber (x)/cucumber-js/features/step_definitions/usage_json_steps.ts:9:18)
    ✔ After # ../../Cucumber x/cucumber-js/src/support_code_library_builder/build_helpers.ts:13

To Reproduce
Steps to reproduce the behavior:

1. Go to 'the root directory for the project'
2. Add a parent directory with a parentheses in its name (include some letters within the parentheses)
3. Open the project in your desired IDE
4. Run the tests
5. See error

Expected behavior
Passing tests

Screenshots

Desktop (please complete the following information):

• OS: macOS Big Sur 11.4
• Version: Main Repository Branch

Additional context
Trying to run tests to contribute to cucumber-js with @mattwynne as part of the Cucumber first timers mob and the folder where I keep my code happens to have parentheses in its name. For more context, this is not the folder shown in the screenshot, that's simply provided for testing purposes.

[mattwynne]

I added some debugging in formatTestCaseAttempt to try and understand how deep the parameter-stripping is being done.

It looks like the stepDefinition message contains a sourceReference that has the stripped-parentheses path:

$ yarn run feature-test features/usage_json_formatter.feature:8 -f message
...
{"stepDefinition":{"id":"1f352a72-138e-4a0d-9bd0-57266210b312","pattern":{"source":"my env includes {string}","type":"CUCUMBER_EXPRESSION"},"sourceReference":{"uri":"../../Cucumber x/cucumber-js/src/support_code_library_builder/build_helpers.ts","location":{"line":13}}}}
...

Notice that the parens have been stripped from the path in the sourceReference in this message.

[mattwynne]

I think I've tracked the problem down to this line:

cucumber-js/src/support_code_library_builder/build_helpers.ts

Line 13 in a3aab75

const stackframes = StackTrace.getSync()

Where we lean on the stacktrace-js library to parse the filename where the step was defined. When I look at what StackTrace.getSync() is returning, the fileName property already has the parentheses stripped from it.

[mattwynne]

Maybe the problem is here?!

[mattwynne]

I think I've found the root cause.

I think it's in the stacktrace.js library's error-stack-parser.

I cloned that library and wrote a failing test for it that reproduces the problem, in a branch which you can see here.

I've made an issue on their issue tracker, so we can continue the discussion over there if you're interested in helping to fix it @eoola? stacktracejs/error-stack-parser#62

[aslakhellesoy]

I think I’ve found another root cause. We’re depending on a micro library that has low activity and release frequency.

Depending on low-frequency libraries is a liability. We should replace it with our own implementation, possibly a function instead of a lib.

[mattwynne]

I thought about this too @aslakhellesoy. Having looked through the code of that library, there's quite a bit of complexity there to deal with handling call stacks in different browsers. So that's the only trade-off with bringing that functionality in-house - Cucumber-JS might not report filename so reliably if run in a browser.

That seems like an edge-case to me, but that's because I only ever use Cucumber-JS in a node/electron process. I'm not sure how much people are running their tests in browsers.

[aslakhellesoy]

We don’t need to worry about running in a browser because we removed support for that in 7.0.0.

[mattwynne]

Good, then I think that settles it, we should rip out stacktrace.js as it's way more than we need.

As you say, we can probably roll our own function using new Error().stack. @eoola if I write you some tests, do you want to try and implement it?

[eoola]

Would love to take that on. Thank you! @mattwynne

[mattwynne]

Here are some things we might be able to lean on:

https://www.npmjs.com/package/v8-callsites
https://github.com/sindresorhus/callsites
https://github.com/tj/callsite

It's currently unclear to me how much of this stuff is now available inside NodeJS itself. I see we're using the CallSite type from NodeJS in our StackTraceFilter already.

My node knowledge isn't deep enough to have a handle on this.

[mattwynne]

OK @eoola this is ready for you.

I've done a bit of "prefactoring" on the current code to make it easier to unit test, and added a unit test for it that helps us to pin-point the problem. The code is in the fix-1735-parens-in-path of this repo.

I'm sorry I don't have time to leave you guidance about how to check out that branch locally - I need to go out now, but I can update this ticket later on with that info.

Once you've checked out that branch locally, you can run the test like this:

/node_modules/.bin/mocha src/support_code_library_builder/get_definition_line_and_uri_spec.ts

If you've got your cucumber-js repo in a parent path without parentheses, that unit test should pass fine. If you run it in a path with parentheses however, it should fail like this:

./node_modules/.bin/mocha src/support_code_library_builder/get_definition_line_and_uri_spec.ts


  !

  0 passing (11ms)
  1 failing

  1) getDefinitionLineAndUri
       correctly gets the filename of the caller:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'internal/timers.js'
- 'src/support_code_library_builder/get_definition_line_and_uri_spec.ts'
      + expected - actual

      -internal/timers.js
      +src/support_code_library_builder/get_definition_line_and_uri_spec.ts
      
      at Context.<anonymous> (/Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /src/support_code_library_builder/get_definition_line_and_uri_spec.ts:8:12)
      at callFn (/Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runnable.js:366:21)
      at Test.Runnable.run (/Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runnable.js:354:5)
      at Runner.runTest (/Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runner.js:680:10)
      at /Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runner.js:803:12
      at next (/Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runner.js:595:14)
      at /Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runner.js:605:7
      at next (/Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runner.js:488:14)
      at Immediate._onImmediate (/Users/matt/git/github.com/cucumber/Cucucmber (JS)
  /node_modules/mocha/lib/runner.js:573:5)
      at processImmediate (internal/timers.js:461:21)

Now for the fix!

I believe that we can simply replace the call to StackTrace.getSync() on line 14 with a call to chain.callSite() from the stack-chain library which we're already using elsewhere in the StackTraceFilter class. I had a play with it and it doesn't seem to have the same problem with parentheses. There's no need for us to have two dependencies that both do the same job, especially if one of them does it poorly.

Once you've done that, you can remove the dependency on stacktrace-js altogether, with yarn remove stacktrace-js and commit the change to package.json and yarn.lock.

Then we'll be ready for your first PR to cucumber-js! 🎉

[mattwynne]

@eoola, @testerfromleic and I mobbed on this today (video) in the new contributors ensemble. We discovered that the chain.callSite() function I'd suggested we use didn't get the line number right - probably because it was ignoring the JavaScript => TypeScript source maps. (Thanks to @romaingweb for suggesting that in the Twitch stream comments!)

We had a go at parsing the stack frame strings ourselves, and we got close to having something working. I'm not sure whether it will work in all environments, like Windows for example.

We might want to try something like https://stackoverflow.com/questions/13227489/how-can-one-get-the-file-path-of-the-caller-function-in-node-js which seems to use native Node/v8 APIs to get the array of CallSite objects.

[mattwynne]

I went down the rabbit hole a little further on the idea of using prepareStackTrace but it seems those CallSite objects return the raw JavaScript line number, without looking up the TypeScript line in the source map.

https://github.com/HoldYourWaffle/ts-call-site seems like it might do the trick.

[mattwynne]

@eoola @davidjgoss and I did some more work on this today: https://www.twitch.tv/videos/1144571453

We tried using prepareStackTrace and got that working, then started exploring how to use the source maps.

[mattwynne]

I made a minimal reproducible example: https://github.com/mattwynne/stacktrace-test

[mattwynne]

I've sent the minimal reproducible example to the ts-node folks: TypeStrong/ts-node#1517

[mattwynne]

We've had a breakthrough! TypeStrong/ts-node#1517 mattwynne/stacktrace-test@e6260e2

[mattwynne]

We've made a draft PR, even though there are still one or two tests failing. We're getting there, and we've removed one of our dependencies (stracktrace-js).

[mattwynne]

Although this closed when we closed the PR, we spotted another problem on Demi's machine with this scenario which also seems to get messed up by parentheses (or maybe spaces? or both?) in the developer's path.

We'll make a separate issue about this.

[mattwynne]

Issue for the other failing test is #1845