diff --git a/CHANGELOG.md b/CHANGELOG.md index da14dd293..ec7f52065 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ Please see [CONTRIBUTING.md](https://github.com/cucumber/cucumber/blob/master/CO ### Added - Cucumber Expressions now support a wider array of parameter types (see [documentation](https://github.com/cucumber/cucumber-expressions#parameter-types)) - Improved styling and usability on report from `html` formatter +- Add a new option to `--format-options`: `printAttachments`. + See [./docs/cli.md#printing-attachments-details](https://github.com/cucumber/cucumber-js/blob/main/docs/cli.md#printing-attachments-details) for more info. + ([#1136](https://github.com/cucumber/cucumber-js/issues/1136) + [#1721](https://github.com/cucumber/cucumber-js/pull/1721)) ### Fixed - Warn users who are on an unsupported node version ([#1922](https://github.com/cucumber/cucumber-js/pull/1922)) diff --git a/docs/cli.md b/docs/cli.md index 6ffccc5c2..e1a0a009f 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -74,6 +74,13 @@ By default, cucumber works in _strict_ mode, meaning it will fail if there are p See [Parallel](./parallel.md). +## Printing Attachments Details + +Printing attachments details can be disabled with +`--fomat-options '{"printAttachments": false}'`. + +This option applies to the progress formatter and the summary formatter. + ## Profiles See [Profiles](./profiles.md). @@ -127,7 +134,7 @@ If you are using [ts-node](https://github.com/TypeStrong/ts-node): --require-module ts-node/register --require 'step-definitions/**/*.ts' ``` -> ⚠️ Some TypeScript setups use `esnext` modules by default, +> ⚠️ Some TypeScript setups use `esnext` modules by default, > which doesn't marry well with Node. You may consider using commonjs instead. > See how to add [extra configuration](#extra-configuration) below. diff --git a/docs/first-time-contributor-tutorial.md b/docs/first-time-contributor-tutorial.md new file mode 100644 index 000000000..255abc1b3 --- /dev/null +++ b/docs/first-time-contributor-tutorial.md @@ -0,0 +1,408 @@ +This tutorial aims to guide a new contributor into fixing the issue #1136 + +PR #1721 has been opened with a new scenario which validate the behavior we +would like to implement. + +That scenario can be found here: [features/error_formatting.feature#L110](features/error_formatting.feature#L110) + +# Addition of unit test + +A new scenario has been added, but we still can add some unit tests to have a +better focus on the pieces of code to modify. + +The first phrase of the issue #1136 references an older PR #1041. That PR has +modified the code in `src/formatter/helpers/issue_helpers.js` and the +corresponding `src/formatter/helpers/issue_helpers_spec.js`. Without digging +deeper in those files and in the PR #1041, if we just take a look at +`issue_helpers_spec.js` - which is actually now a Typescript file +[issue_helpers_spec.ts](src/formatter/helpers/issue_helpers_spec.ts) - we can +see some tests that seems to be related to our issue. + +So, let's move at the last `describe` section of `issue_helpers_spec.ts`, and +make it looks like the following: + +```typescript +// src/formatter/helpers/issue_helpers_spec.ts + + describe('step with attachment text', () => { + it('prints the scenario', async () => { + // here's the content of the "prints the scenario" test + }) + + describe('when it is requested to not print attachments', () => { + it('does not output attachment', async () => { + // Arrange + const sourceData = reindent(` + Feature: my feature + Scenario: my scenario + Given attachment step1 + When attachment step2 + Then a passing step + `) + + // Act + const output = await testFormatIssue(sourceData) + + // Assert + expect(output).to.eql( + reindent(` + 1) Scenario: my scenario # a.feature:2 + ${figures.tick} Given attachment step1 # steps.ts:35 + ${figures.cross} When attachment step2 # steps.ts:41 + error + - Then a passing step # steps.ts:29 + + + `) + ) + }) + }) + }) +``` + +We notice here that `it('does not output attachment')` looks a lot like +`it('prints the scenario')`. One difference is that the expected output does not +print any info regarding the attachments anymore. + +Now execute the unit tests: + + yarn run unit-test + +As expected, our new test is failing. Now let's add a new parameter to +`testFormatIssue`: + +Line 10 of `issue_helpers_spec.ts`, add a new `printAttachments` parameter to +`testFormatIssue` with a default value to `true` to avoid breaking things +outside of our new test: + +```typescript +async function testFormatIssue(sourceData: string, printAttachments = true): Promise { +``` + +In our new test, we can call `testFormatIssue` with `printAttachments` parameter +set to `false`: + +```typescript +// src/formatter/helpers/issue_helpers_spec.ts + + it('does not output attachment', async () => { + // Arrange + const sourceData = reindent(` + Feature: my feature + Scenario: my scenario + Given attachment step1 + When attachment step2 + Then a passing step + `) + + // Act + const output = await testFormatIssue(sourceData, false) + + // Assert + expect(output).to.eql( + reindent(` + 1) Scenario: my scenario # a.feature:2 + ${figures.tick} Given attachment step1 # steps.ts:35 + ${figures.cross} When attachment step2 # steps.ts:41 + error + - Then a passing step # steps.ts:29 + + + `) + ) + }) +``` + +Our test is still failing - that is expected - but we have some tests now. + +# Fixing the code + +Still in `issue_helpers_spec.ts`, if we look at `testFormatIssue`, we can see +that it calls a `formatIssue` method which is imported from `issues_helper.ts`. + +Open `issue_helper.ts` and look at the `formatIssue` method. It lead us into +a method named `formatTestCaseAttempt` which is defined in +`test_case_attempt_formatter.ts`. One step further and we find that `formatStep` +method, still in `test_case_attempt_formatter.ts`, which seems responsible for +actually adding the attachments to the output. So, let's give it a try: change +the `formatStep` method like the following: + +```typescript +// src/formatter/helpers/test_case_attempt_formatter.ts + +interface IFormatStepRequest { + colorFns: IColorFns + testStep: IParsedTestStep + printAttachments?: boolean +} + +function formatStep({ + colorFns, + testStep, + printAttachments, +}: IFormatStepRequest): string { +``` + +Here we have added a `printAttachments` parameter. This parameter is optional +in order to avoid breaking things. Now look for the block which process the +attachments, and make sure to execute it only if `printAttachments` is not +explicitly false: + +```typescript +// src/formatter/helpers/test_case_attempt_formatter.ts#formatStep + + if (valueOrDefault(printAttachments, true)) { + attachments.forEach(({ body, mediaType }) => { + const message = mediaType === 'text/plain' ? `: ${body}` : '' + text += indentString(`Attachment (${mediaType})${message}\n`, 4) + }) + } +``` + +Now we have to make sure that the value we pass to `testFormatIssue` is +transmitted to `formatStep`. That means updating `formatTestCaseAttempt`: +```typescript +// src/formatter/helpers/test_case_attempt_formatter.ts + +export interface IFormatTestCaseAttemptRequest { + colorFns: IColorFns + cwd: string + testCaseAttempt: ITestCaseAttempt + snippetBuilder: StepDefinitionSnippetBuilder + supportCodeLibrary: ISupportCodeLibrary + printAttachments?: boolean +} + +export function formatTestCaseAttempt({ + colorFns, + cwd, + snippetBuilder, + supportCodeLibrary, + testCaseAttempt, + printAttachments, +}: IFormatTestCaseAttemptRequest): string { + + // locate the call to formatStep to add the argument: + + text += formatStep({ colorFns, testStep, printAttachments }) +``` + +and `formatIssue` in `issue_helpers.ts`: + +```typescript +// src/formatter/helpers/issue_helpers.ts + +export interface IFormatIssueRequest { + colorFns: IColorFns + cwd: string + number: number + snippetBuilder: StepDefinitionSnippetBuilder + testCaseAttempt: ITestCaseAttempt + supportCodeLibrary: ISupportCodeLibrary + printAttachments?: boolean +} + +export function formatIssue({ + colorFns, + cwd, + number, + snippetBuilder, + testCaseAttempt, + supportCodeLibrary, + printAttachments, +}: IFormatIssueRequest): string { + + // lcoate the call to formatTestCaseAttempt to add the argument: + + const formattedTestCaseAttempt = formatTestCaseAttempt({ + colorFns, + cwd, + snippetBuilder, + testCaseAttempt, + supportCodeLibrary, + printAttachments, + }) + +``` + +And, finally, in `issue_helpers_spec.ts`, within `testFormatIssue`: +```typescript +// src/formatter/helpers/issue_helpers_spec.ts + + return formatIssue({ + cwd: 'project/', + colorFns: getColorFns(false), + number: 1, + snippetBuilder: FormatterBuilder.getStepDefinitionSnippetBuilder({ + cwd: 'project/', + supportCodeLibrary, + }), + supportCodeLibrary, + testCaseAttempt, + printAttachments, + }) +``` + +Now, all unit tests should pass: + + yarn run unit-test + +But, as expected, our acceptance test is still failing because we did not yet +managed the cli options: + + yarn run feature-test + +# Make the feature test to pass + +Our formatter has been fixed. Now it is time to add the option to the CLI. + +If we look for `formatOptions` in the code base, we find `src/cli/argv_parser.ts` +which looks like a good candidate. It describes the interfaces for the CLI options. + +We can easily spot the `IParsedArgvFormatOptions` interface which describe the options +available for `--format-options`. Let's add our `printAttachments` here: + +```typescript +// src/cli/argv_parser.ts + +export interface IParsedArgvFormatOptions { + colorsEnabled?: boolean + rerun?: IParsedArgvFormatRerunOptions + snippetInterface?: SnippetInterface + snippetSyntax?: string + printAttachments?: boolean + [customKey: string]: any +} +``` + +Next step: adding this option to our formatters. + +All formatters extends the `Formatter` base class, so let's add the option there: + +```typescript +// src/formatter/index.ts + +// some imports +import { valueOrDefault } from '../value_checker' + +// ... + +export default class Formatter { + protected colorFns: IColorFns + protected cwd: string + protected eventDataCollector: EventDataCollector + protected log: IFormatterLogFn + protected snippetBuilder: StepDefinitionSnippetBuilder + protected stream: WritableStream + protected supportCodeLibrary: ISupportCodeLibrary + protected printAttachments: boolean + private readonly cleanup: IFormatterCleanupFn + + constructor(options: IFormatterOptions) { + this.colorFns = options.colorFns + this.cwd = options.cwd + this.eventDataCollector = options.eventDataCollector + this.log = options.log + this.snippetBuilder = options.snippetBuilder + this.stream = options.stream + this.supportCodeLibrary = options.supportCodeLibrary + this.cleanup = options.cleanup + this.printAttachments = valueOrDefault( + options.parsedArgvOptions.printAttachments, + true + ) + } +``` + +We have added the option `printAttachments` to the formatter members. We make sure +it is `true` per default using the `valueOrDefault` function. Otherwise, if the option +is not specified, it would have been falsy per default. + +Finally, we have to make sure we call the `formatIssue` method with the value of +`printAttachments`. + +A global search for `formatIssue` find two usage for it: in `progress_bar_formatter.ts` +and in `summary_formatter.ts`. Let's update those two files: + +```typescript +// src/formatter/progress_bar_formatter.ts + +// Locate the call to formatIssue + + this.progressBar.interrupt( + formatIssue({ + colorFns: this.colorFns, + cwd: this.cwd, + number: this.issueCount, + snippetBuilder: this.snippetBuilder, + supportCodeLibrary: this.supportCodeLibrary, + testCaseAttempt, + printAttachments: this.printAttachments, + }) + ) +``` + +```typescript +// src/formatter/summary_formatter.ts + +// Locate the call to formatIssue + + logIssues({ issues, title }: ILogIssuesRequest): void { + this.log(`${title}:\n\n`) + issues.forEach((testCaseAttempt, index) => { + this.log( + formatIssue({ + colorFns: this.colorFns, + cwd: this.cwd, + number: index + 1, + snippetBuilder: this.snippetBuilder, + supportCodeLibrary: this.supportCodeLibrary, + testCaseAttempt, + printAttachments: this.printAttachments, + }) + ) + }) + } +``` + +So, it should be working now + + yarn run feature-test + +# Finalizing the Pull Request + +Last but not least, let's make sure the PR is valid. + +First, execute all the tests: + + yarn test + +You may have some linting issues here. Fix those and retry until everything pass. + +We also need to add some documentation in `docs/cli.md`. Add the following between +the `## Parallel` and the `## Profiles` sections: + +```markdown + +## Printing Attachments Details + +Printing attachments details can be disabled with +`--fomat-options '{"printAttachmants": false}'`. + +This option applies to the progress formatter and the summary formatter. + +``` + +Now we should add an entry in the CHANGELOG.md. Open it, locate the `### Added` section +for the unreleased changes, and add somethig about the changes we just did: + +```markdown + +* Add a new option to `--format-option`: `printAttachments`. + See [./docs/cli.md#printing-attachments-details](https://github.com/cucumber/cucumber-js/blob/main/docs/cli.md#printing-attachments-details) for more info. + ([#1136](https://github.com/cucumber/cucumber-js/issues/1136) + [#1721](https://github.com/cucumber/cucumber-js/pull/1721)) + +``` + +That's it! You can now set your PR as ready for review! \ No newline at end of file diff --git a/docs/formatters.md b/docs/formatters.md index 040a8f9f0..08bf6ffee 100644 --- a/docs/formatters.md +++ b/docs/formatters.md @@ -29,7 +29,8 @@ This option is repeatable, so you can use it multiple times and the objects will Some options offered by built-in formatters: -- `colorsEnabled` - if set to `false`, colors in terminal output are disabled. +- `colorsEnabled` - if set to `false`, colors in terminal output are disabled +- `printAttachments` - if set to `false`, attachments won't be part of progress bars and summary reports ## Built-in formatters diff --git a/features/error_formatting.feature b/features/error_formatting.feature index 4fe7c6b4c..60251bc5e 100644 --- a/features/error_formatting.feature +++ b/features/error_formatting.feature @@ -106,3 +106,31 @@ Feature: Error formatting Pending """ And it fails + + Scenario: failing scenario when requested to not print step attachments + Given a file named "features/a.feature" with: + """ + Feature: some feature + + Scenario: some scenario + Given a basic step + And a pending step + """ + And a file named "features/step_definitions/cucumber_steps.js" with: + """ + const {Given} = require('@cucumber/cucumber') + + Given(/^a basic step$/, function() { this.attach('Some attached text.') }) + Given(/^a pending step$/, function() { return 'pending' }) + """ + When I run cucumber-js with `--format-options '{"printAttachments": false}'` + Then the output contains the text: + """ + Warnings: + + 1) Scenario: some scenario # features/a.feature:3 + ✔ Given a basic step # features/step_definitions/cucumber_steps.js:3 + ? And a pending step # features/step_definitions/cucumber_steps.js:4 + Pending + """ + And it fails diff --git a/src/cli/argv_parser.ts b/src/cli/argv_parser.ts index f908dbf41..2f6360e17 100644 --- a/src/cli/argv_parser.ts +++ b/src/cli/argv_parser.ts @@ -16,6 +16,7 @@ export interface IParsedArgvFormatOptions { rerun?: IParsedArgvFormatRerunOptions snippetInterface?: SnippetInterface snippetSyntax?: string + printAttachments?: boolean [customKey: string]: any } diff --git a/src/formatter/helpers/issue_helpers.ts b/src/formatter/helpers/issue_helpers.ts index 02fa6f419..736bda7bb 100644 --- a/src/formatter/helpers/issue_helpers.ts +++ b/src/formatter/helpers/issue_helpers.ts @@ -37,6 +37,7 @@ export interface IFormatIssueRequest { snippetBuilder: StepDefinitionSnippetBuilder testCaseAttempt: ITestCaseAttempt supportCodeLibrary: ISupportCodeLibrary + printAttachments?: boolean } export function formatIssue({ @@ -46,6 +47,7 @@ export function formatIssue({ snippetBuilder, testCaseAttempt, supportCodeLibrary, + printAttachments = true, }: IFormatIssueRequest): string { const prefix = `${number.toString()}) ` const formattedTestCaseAttempt = formatTestCaseAttempt({ @@ -54,6 +56,7 @@ export function formatIssue({ snippetBuilder, testCaseAttempt, supportCodeLibrary, + printAttachments, }) const lines = formattedTestCaseAttempt.split('\n') const updatedLines = lines.map((line, index) => { diff --git a/src/formatter/helpers/issue_helpers_spec.ts b/src/formatter/helpers/issue_helpers_spec.ts index f60cfed10..c5a06f23c 100644 --- a/src/formatter/helpers/issue_helpers_spec.ts +++ b/src/formatter/helpers/issue_helpers_spec.ts @@ -8,7 +8,10 @@ import { reindent } from 'reindent-template-literals' import { getBaseSupportCodeLibrary } from '../../../test/fixtures/steps' import FormatterBuilder from '../builder' -async function testFormatIssue(sourceData: string): Promise { +async function testFormatIssue( + sourceData: string, + printAttachments: boolean = true +): Promise { const sources = [ { data: sourceData, @@ -30,6 +33,7 @@ async function testFormatIssue(sourceData: string): Promise { }), supportCodeLibrary, testCaseAttempt, + printAttachments, }) } @@ -266,6 +270,35 @@ describe('IssueHelpers', () => { `) ) }) + + describe('when it is requested to not print attachments', () => { + it('does not output attachment', async () => { + // Arrange + const sourceData = reindent(` + Feature: my feature + Scenario: my scenario + Given attachment step1 + When attachment step2 + Then a passing step + `) + + // Act + const output = await testFormatIssue(sourceData, false) + + // Assert + expect(output).to.eql( + reindent(` + 1) Scenario: my scenario # a.feature:2 + ${figures.tick} Given attachment step1 # steps.ts:35 + ${figures.cross} When attachment step2 # steps.ts:41 + error + - Then a passing step # steps.ts:29 + + + `) + ) + }) + }) }) }) }) diff --git a/src/formatter/helpers/test_case_attempt_formatter.ts b/src/formatter/helpers/test_case_attempt_formatter.ts index 3c67c3cb0..648e03d11 100644 --- a/src/formatter/helpers/test_case_attempt_formatter.ts +++ b/src/formatter/helpers/test_case_attempt_formatter.ts @@ -40,9 +40,14 @@ function getStepMessage(testStep: IParsedTestStep): string { interface IFormatStepRequest { colorFns: IColorFns testStep: IParsedTestStep + printAttachments?: boolean } -function formatStep({ colorFns, testStep }: IFormatStepRequest): string { +function formatStep({ + colorFns, + testStep, + printAttachments, +}: IFormatStepRequest): string { const { result: { status }, actionLocation, @@ -59,10 +64,12 @@ function formatStep({ colorFns, testStep }: IFormatStepRequest): string { const argumentsText = formatStepArgument(testStep.argument) text += indentString(`${colorFn(argumentsText)}\n`, 4) } - attachments.forEach(({ body, mediaType }) => { - const message = mediaType === 'text/plain' ? `: ${body}` : '' - text += indentString(`Attachment (${mediaType})${message}\n`, 4) - }) + if (valueOrDefault(printAttachments, true)) { + attachments.forEach(({ body, mediaType }) => { + const message = mediaType === 'text/plain' ? `: ${body}` : '' + text += indentString(`Attachment (${mediaType})${message}\n`, 4) + }) + } const message = getStepMessage(testStep) if (message !== '') { text += `${indentString(colorFn(message), 4)}\n` @@ -76,6 +83,7 @@ export interface IFormatTestCaseAttemptRequest { testCaseAttempt: ITestCaseAttempt snippetBuilder: StepDefinitionSnippetBuilder supportCodeLibrary: ISupportCodeLibrary + printAttachments?: boolean } export function formatTestCaseAttempt({ @@ -84,6 +92,7 @@ export function formatTestCaseAttempt({ snippetBuilder, supportCodeLibrary, testCaseAttempt, + printAttachments, }: IFormatTestCaseAttemptRequest): string { const parsed = parseTestCaseAttempt({ cwd, @@ -97,7 +106,7 @@ export function formatTestCaseAttempt({ formatLocation(parsed.testCase.sourceLocation) )}\n` parsed.testSteps.forEach((testStep) => { - text += formatStep({ colorFns, testStep }) + text += formatStep({ colorFns, testStep, printAttachments }) }) return `${text}\n` } diff --git a/src/formatter/index.ts b/src/formatter/index.ts index 16ff0ad28..015302f02 100644 --- a/src/formatter/index.ts +++ b/src/formatter/index.ts @@ -8,6 +8,7 @@ import { WriteStream as TtyWriteStream } from 'tty' import { EventEmitter } from 'events' import { IParsedArgvFormatOptions } from '../cli/argv_parser' import HttpStream from './http_stream' +import { valueOrDefault } from '../value_checker' export type IFormatterStream = | FsWriteStream @@ -38,6 +39,7 @@ export default class Formatter { protected snippetBuilder: StepDefinitionSnippetBuilder protected stream: WritableStream protected supportCodeLibrary: ISupportCodeLibrary + protected printAttachments: boolean private readonly cleanup: IFormatterCleanupFn static readonly documentation: string @@ -50,6 +52,10 @@ export default class Formatter { this.stream = options.stream this.supportCodeLibrary = options.supportCodeLibrary this.cleanup = options.cleanup + this.printAttachments = valueOrDefault( + options.parsedArgvOptions.printAttachments, + true + ) } async finished(): Promise { diff --git a/src/formatter/json_formatter.ts b/src/formatter/json_formatter.ts index cb1396f1a..1f758cc05 100644 --- a/src/formatter/json_formatter.ts +++ b/src/formatter/json_formatter.ts @@ -329,7 +329,7 @@ export default class JsonFormatter extends Formatter { feature: messages.Feature, scenario: messages.Scenario ): IJsonTag { - const byAstNodeId = (tag: messages.Tag): Boolean => + const byAstNodeId = (tag: messages.Tag): boolean => tag.id === tagData.astNodeId const flatten = ( acc: messages.Tag[], diff --git a/src/formatter/progress_bar_formatter.ts b/src/formatter/progress_bar_formatter.ts index 660b3994f..54226db80 100644 --- a/src/formatter/progress_bar_formatter.ts +++ b/src/formatter/progress_bar_formatter.ts @@ -80,6 +80,7 @@ export default class ProgressBarFormatter extends Formatter { snippetBuilder: this.snippetBuilder, supportCodeLibrary: this.supportCodeLibrary, testCaseAttempt, + printAttachments: this.printAttachments, }) ) if (testCaseFinished.willBeRetried) { diff --git a/src/formatter/summary_formatter.ts b/src/formatter/summary_formatter.ts index f91034a3d..3c6937ce3 100644 --- a/src/formatter/summary_formatter.ts +++ b/src/formatter/summary_formatter.ts @@ -88,6 +88,7 @@ export default class SummaryFormatter extends Formatter { snippetBuilder: this.snippetBuilder, supportCodeLibrary: this.supportCodeLibrary, testCaseAttempt, + printAttachments: this.printAttachments, }) ) })