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

alternative stack trace approach #2119

Merged
merged 16 commits into from
Nov 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
Please see [CONTRIBUTING.md](https://github.com/cucumber/cucumber/blob/master/CONTRIBUTING.md) on how to contribute to Cucumber.

## [Unreleased]
## Changed
- Handle stack traces without V8-specific modification ([#2119](https://github.com/cucumber/cucumber-js/pull/2119))

## [8.7.0] - 2022-10-17
### Deprecated
Expand Down
11 changes: 11 additions & 0 deletions docs/transpiling.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,14 @@ If you are using babel with [@babel/preset-typescript](https://babeljs.io/docs/e
### ESM

See [ESM](./esm.md) for general advice on using loaders for transpilation in ESM projects.

### Source maps

Source maps are used to ensure accurate source references and stack traces in Cucumber's reporting, by giving traceability from a transpiled piece of code back to the original source code.

Just-in-time transpilers like `ts-node` and `@babel/register` have sensible default configuration that emits source maps and enables them in the runtime environment, so you shouldn't have to do anything in order for source maps to work.

If you're using step definition code that's _already_ transpiled (maybe because it's a shared library) then you'll need to:

1. Ensure source maps are emitted by your transpiler. You can verify by checking for a comment starting with `//# sourceMappingURL=` at the end of your transpiled file(s).
2. Ensure source maps are enabled at runtime. Node.js supports this natively via [the `--enable-source-maps` flag](https://nodejs.org/docs/latest/api/cli.html#--enable-source-maps).
51 changes: 51 additions & 0 deletions features/stack_traces.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
@spawn
@source-mapping
Feature: Stack traces
Background:
Given a file named "features/a.feature" with:
"""
Feature: some feature
Scenario: some scenario
Given a passing step
And a failing step
"""

Rule: Source maps are respected when dealing with transpiled support code

Just-in-time transpilers like `@babel/register` and `ts-node` emit source maps with
the transpiled code. Cucumber users expect stack traces to point to the line and column
in the original source file when there is an error.

Background:
Given a file named "features/steps.ts" with:
"""
import { Given } from '@cucumber/cucumber'

interface Something {
field1: string
field2: string
}

Given('a passing step', function() {})

Given('a failing step', function() {
throw new Error('boom')
})
"""

Scenario: commonjs
When I run cucumber-js with `--require-module ts-node/register --require features/steps.ts`
Then the output contains the text:
"""
/features/steps.ts:11:9
"""
And it fails

Scenario: esm
Given my env includes "{\"NODE_OPTIONS\":\"--loader ts-node/esm\"}"
When I run cucumber-js with `--import features/steps.ts`
Then the output contains the text:
"""
/features/steps.ts:11:9
"""
And it fails
12 changes: 8 additions & 4 deletions features/support/world.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,14 @@ export class World {
parseEnvString(str: string): NodeJS.ProcessEnv {
const result: NodeJS.ProcessEnv = {}
if (doesHaveValue(str)) {
str
.split(/\s+/)
.map((keyValue) => keyValue.split('='))
.forEach((pair) => (result[pair[0]] = pair[1]))
try {
Object.assign(result, JSON.parse(str))
} catch {
str
.split(/\s+/)
.map((keyValue) => keyValue.split('='))
.forEach((pair) => (result[pair[0]] = pair[1]))
}
}
return result
}
Expand Down
49 changes: 36 additions & 13 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@
"node": "12 || 14 || 16 || 17 || 18"
},
"dependencies": {
"@cspotcode/source-map-support": "^0.8.0",
"@cucumber/ci-environment": "9.1.0",
"@cucumber/cucumber-expressions": "16.0.0",
"@cucumber/gherkin": "24.1.0",
Expand All @@ -213,6 +212,7 @@
"debug": "^4.3.4",
"duration": "^0.2.2",
"durations": "^3.4.2",
"error-stack-parser": "^2.1.4",
"figures": "^3.2.0",
"glob": "^7.1.6",
"has-ansi": "^4.0.1",
Expand All @@ -226,7 +226,6 @@
"progress": "^2.0.3",
"resolve-pkg": "^2.0.0",
"semver": "7.3.8",
"stack-chain": "^2.0.0",
"string-argv": "^0.3.1",
"strip-ansi": "6.0.1",
"supports-color": "^8.1.1",
Expand Down Expand Up @@ -311,7 +310,7 @@
"prepublishOnly": "rm -rf lib && npm run build-local",
"pretest-coverage": "npm run build-local",
"pretypes-test": "npm run build-local",
"test-coverage": "nyc --silent mocha 'src/**/*_spec.ts' 'compatibility/**/*_spec.ts' && nyc --silent --no-clean node bin/cucumber.js && nyc report --reporter=lcov",
"test-coverage": "nyc --silent mocha 'src/**/*_spec.ts' 'compatibility/**/*_spec.ts' && nyc --silent --no-clean node bin/cucumber.js --tags \"not @source-mapping\" && nyc report --reporter=lcov",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nyc instrumentation seems to mess these scenarios up when we check for coverage. We could try and fix up the environment for the child process but it didn't feel worth it.

"test": "npm run lint && npm run types-test && npm run unit-test && npm run cck-test && npm run feature-test",
"types-test": "tsd",
"unit-test": "mocha 'src/**/*_spec.ts'",
Expand Down
38 changes: 38 additions & 0 deletions src/filter_stack_trace.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import path from 'path'
import { valueOrDefault } from './value_checker'
import { StackFrame } from 'error-stack-parser'

const projectRootPath = path.join(__dirname, '..')
const projectChildDirs = ['src', 'lib', 'node_modules']

export function isFileNameInCucumber(fileName: string): boolean {
return projectChildDirs.some((dir) =>
fileName.startsWith(path.join(projectRootPath, dir))
)
}

export function filterStackTrace(frames: StackFrame[]): StackFrame[] {
if (isErrorInCucumber(frames)) {
return frames
}
const index = frames.findIndex((x) => isFrameInCucumber(x))
if (index === -1) {
return frames
}
return frames.slice(0, index)
}

function isErrorInCucumber(frames: StackFrame[]): boolean {
const filteredFrames = frames.filter((x) => !isFrameInNode(x))
return filteredFrames.length > 0 && isFrameInCucumber(filteredFrames[0])
}

function isFrameInCucumber(frame: StackFrame): boolean {
const fileName = valueOrDefault(frame.getFileName(), '')
return isFileNameInCucumber(fileName)
}

function isFrameInNode(frame: StackFrame): boolean {
const fileName = valueOrDefault(frame.getFileName(), '')
return !fileName.includes(path.sep)
}
22 changes: 22 additions & 0 deletions src/runtime/format_error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { format } from 'assertion-error-formatter'
import errorStackParser from 'error-stack-parser'
import { filterStackTrace } from '../filter_stack_trace'

export function formatError(error: Error, filterStackTraces: boolean): string {
let filteredStack: string
if (filterStackTraces) {
try {
filteredStack = filterStackTrace(errorStackParser.parse(error))
.map((f) => f.source)
.join('\n')
} catch {
// if we weren't able to parse and filter, we'll settle for the original
}
}
return format(error, {
colorFns: {
errorStack: (stack: string) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of abusing the assertion-error-formatter API here, which doesn't feel great. It could maybe do with a formatErrorStack option where you can provide a function that rewrites the stack as desired?

filteredStack ? `\n${filteredStack}` : stack,
},
})
}
10 changes: 1 addition & 9 deletions src/runtime/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import * as messages from '@cucumber/messages'
import { IdGenerator } from '@cucumber/messages'
import { EventEmitter } from 'events'
import { EventDataCollector } from '../formatter/helpers'
import StackTraceFilter from '../stack_trace_filter'
import { ISupportCodeLibrary } from '../support_code_library_builder/types'
import { assembleTestCases } from './assemble_test_cases'
import { retriesForPickle, shouldCauseFailure } from './helpers'
Expand Down Expand Up @@ -40,7 +39,6 @@ export default class Runtime implements IRuntime {
private readonly newId: IdGenerator.NewId
private readonly options: IRuntimeOptions
private readonly pickleIds: string[]
private readonly stackTraceFilter: StackTraceFilter
private readonly supportCodeLibrary: ISupportCodeLibrary
private success: boolean
private runTestRunHooks: RunsTestRunHooks
Expand All @@ -59,7 +57,6 @@ export default class Runtime implements IRuntime {
this.newId = newId
this.options = options
this.pickleIds = pickleIds
this.stackTraceFilter = new StackTraceFilter()
this.supportCodeLibrary = supportCodeLibrary
this.success = true
this.runTestRunHooks = makeRunTestRunHooks(
Expand All @@ -85,6 +82,7 @@ export default class Runtime implements IRuntime {
testCase,
retries,
skip,
filterStackTraces: this.options.filterStacktraces,
supportCodeLibrary: this.supportCodeLibrary,
worldParameters: this.options.worldParameters,
})
Expand All @@ -95,9 +93,6 @@ export default class Runtime implements IRuntime {
}

async start(): Promise<boolean> {
if (this.options.filterStacktraces) {
this.stackTraceFilter.filter()
}
const testRunStarted: messages.Envelope = {
testRunStarted: {
timestamp: this.stopwatch.timestamp(),
Expand Down Expand Up @@ -132,9 +127,6 @@ export default class Runtime implements IRuntime {
},
}
this.eventBroadcaster.emit('envelope', testRunFinished)
if (this.options.filterStacktraces) {
this.stackTraceFilter.unfilter()
}
return this.success
}
}
Loading