Skip to content

Commit d845e32

Browse files
vincent-psargaaurelien-reevesAslak Hellesøyvincentcapicotto
authored
Display error banner when Reports server returns status code ≠ 2xx (cucumber#1608)
* Add scenario displaying expected result * Display error returned by the server instead of stack trace Update scenario to ensure cucumber execution fails if the server returned an error * Make cucumber-js fail when an error is returned by the server * Rearrange steps like other scenarios * Disable fail-fast for CI * Make error first arg in HttpStream to match conventions * Transform HttpStream from Writable to Transform * Get one more test passing with new Stream * Refactoring. Next up: Remove sendRequest recursion * Refactor HttpStream to avoid recursion Get banner displayed as expected * Simplify HttpStream implementation * Expect HTTP error in output * Use progress formatter in CI Co-authored-by: aurelien-reeves <[email protected]> Co-authored-by: Aslak Hellesøy <[email protected]> Co-authored-by: vincent.capicotto <[email protected]>
1 parent 540b4b7 commit d845e32

File tree

11 files changed

+241
-128
lines changed

11 files changed

+241
-128
lines changed

‎.github/workflows/build.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ jobs:
1414
- ubuntu-latest
1515
- windows-latest
1616
node-version: [10.x, 12.x, 14.x, 15.x]
17+
fail-fast: false
18+
1719
steps:
1820
- uses: actions/checkout@v2
1921
- name: with Node.js ${{ matrix.node-version }} on ${{ matrix.os }}

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ Please see [CONTRIBUTING.md](https://github.com/cucumber/cucumber/blob/master/CO
2222
### Fixed
2323

2424
* Fix types for hook functions so they can return e.g. `'skipped'` ([#1542](https://github.com/cucumber/cucumber-js/pull/1542))
25+
* Display the response of the reports server when an error is returned before failing. ([#1608](https://github.com/cucumber/cucumber-js/pull/1608))
2526

2627
## [7.0.0] (2020-12-21)
2728

‎cucumber.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
const feature = [
22
'--require-module ts-node/register',
33
'--require features/**/*.ts',
4-
`--format ${
5-
process.env.CI || !process.stdout.isTTY ? 'progress' : 'progress-bar'
6-
}`,
4+
`--format progress-bar`,
75
'--format rerun:@rerun.txt',
86
'--format usage:usage.txt',
97
'--format message:messages.ndjson',

‎features/publish.feature

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ Feature: Publish reports
5454

5555
@spawn
5656
Scenario: Report is published when CUCUMBER_PUBLISH_TOKEN is set
57-
When I run cucumber-js with arguments `` and env `CUCUMBER_PUBLISH_TOKEN=keyboardcat`
57+
When I run cucumber-js with arguments `` and env `CUCUMBER_PUBLISH_TOKEN=f318d9ec-5a3d-4727-adec-bd7b69e2edd3`
5858
Then it passes
5959
And the server should receive the following message types:
6060
| meta |
@@ -69,7 +69,7 @@ Feature: Publish reports
6969
| testStepFinished |
7070
| testCaseFinished |
7171
| testRunFinished |
72-
And the server should receive an "Authorization" header with value "Bearer keyboardcat"
72+
And the server should receive an "Authorization" header with value "Bearer f318d9ec-5a3d-4727-adec-bd7b69e2edd3"
7373

7474
@spawn
7575
Scenario: a banner is displayed after publication
@@ -101,6 +101,20 @@ Feature: Publish reports
101101
│ module.exports = { default: '--publish-quiet' } │
102102
└──────────────────────────────────────────────────────────────────────────┘
103103
"""
104+
105+
@spawn
106+
Scenario: when results are not published due to an error raised by the server, the banner is displayed
107+
When I run cucumber-js with env `CUCUMBER_PUBLISH_TOKEN=keyboardcat`
108+
Then it fails
109+
And the error output contains the text:
110+
"""
111+
┌─────────────────────┐
112+
│ Error invalid token │
113+
└─────────────────────┘
114+
115+
Unexpected http status 401 from GET http://localhost:9987
116+
"""
117+
104118
@spawn
105119
Scenario: the publication banner is not shown when publication is done
106120
When I run cucumber-js with arguments `<args>` and env `<env>`
@@ -110,10 +124,10 @@ Feature: Publish reports
110124
"""
111125

112126
Examples:
113-
| args | env |
114-
| --publish | |
115-
| | CUCUMBER_PUBLISH_ENABLED=true |
116-
| | CUCUMBER_PUBLISH_TOKEN=123456 |
127+
| args | env |
128+
| --publish | |
129+
| | CUCUMBER_PUBLISH_ENABLED=true |
130+
| | CUCUMBER_PUBLISH_TOKEN=f318d9ec-5a3d-4727-adec-bd7b69e2edd3 |
117131

118132
@spawn
119133
Scenario: the publication banner is not shown when publication is disabled

‎features/step_definitions/cli_steps.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,15 @@ When(
3737
}
3838
)
3939

40+
When(
41+
/^I run cucumber-js with env `(|.+)`$/,
42+
{ timeout: 10000 },
43+
async function (this: World, envString: string) {
44+
const env = this.parseEnvString(envString)
45+
return await this.run(this.localExecutablePath, [], env)
46+
}
47+
)
48+
4049
When(
4150
/^I run cucumber-js with all formatters(?: and `(|.+)`)?$/,
4251
{ timeout: 10000 },
@@ -67,10 +76,14 @@ When(
6776
Then(/^it passes$/, () => {}) // eslint-disable-line @typescript-eslint/no-empty-function
6877

6978
Then(/^it fails$/, function (this: World) {
70-
const actualCode = doesHaveValue(this.lastRun.error)
79+
const actualCode: number = doesHaveValue(this.lastRun.error)
7180
? this.lastRun.error.code
7281
: 0
73-
expect(actualCode).not.to.eql(0)
82+
83+
expect(actualCode).not.to.eql(
84+
0,
85+
`Expected non-zero exit status, but got ${actualCode}`
86+
)
7487
this.verifiedLastRunError = true
7588
})
7689

‎features/support/hooks.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,11 @@ Before('@global-install', function (this: World) {
9494
)
9595
})
9696

97-
After(function (this: World) {
97+
After(async function (this: World) {
98+
if (this.reportServer?.started) {
99+
await this.reportServer.stop()
100+
}
101+
98102
if (
99103
doesHaveValue(this.lastRun) &&
100104
doesHaveValue(this.lastRun.error) &&
@@ -106,9 +110,3 @@ After(function (this: World) {
106110
)
107111
}
108112
})
109-
110-
After(async function (this: World) {
111-
if (this.reportServer?.started) {
112-
await this.reportServer.stop()
113-
}
114-
})

‎package.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,9 @@
279279
},
280280
"tsd": {
281281
"compilerOptions": {
282-
"types": ["long"]
282+
"types": [
283+
"long"
284+
]
283285
}
284286
},
285287
"bin": {

‎src/cli/index.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import GherkinStreams from '@cucumber/gherkin/dist/src/stream/GherkinStreams'
2828
import { ISupportCodeLibrary } from '../support_code_library_builder/types'
2929
import { IParsedArgvFormatOptions } from './argv_parser'
3030
import HttpStream from '../formatter/http_stream'
31+
import { Writable } from 'stream'
3132

3233
const { incrementing, uuid } = IdGenerator
3334

@@ -98,14 +99,26 @@ export default class Cli {
9899
headers.Authorization = `Bearer ${process.env.CUCUMBER_PUBLISH_TOKEN}`
99100
}
100101

101-
stream = new HttpStream(outputTo, 'GET', headers, (content) =>
102-
console.error(content)
103-
)
102+
stream = new HttpStream(outputTo, 'GET', headers)
103+
const readerStream = new Writable({
104+
objectMode: true,
105+
write: function (responseBody: string, encoding, writeCallback) {
106+
console.error(responseBody)
107+
writeCallback()
108+
},
109+
})
110+
stream.pipe(readerStream)
104111
} else {
105112
const fd = await fs.open(path.resolve(this.cwd, outputTo), 'w')
106113
stream = fs.createWriteStream(null, { fd })
107114
}
108115
}
116+
117+
stream.on('error', (error) => {
118+
console.error(error.message)
119+
process.exit(1)
120+
})
121+
109122
const typeOptions = {
110123
cwd: this.cwd,
111124
eventBroadcaster,

‎src/formatter/http_stream.ts

Lines changed: 82 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,11 @@
1-
import { pipeline, Writable } from 'stream'
1+
import { pipeline, Transform, Writable } from 'stream'
22
import tmp from 'tmp'
33
import fs from 'fs'
44
import http from 'http'
55
import https from 'https'
66
import { doesHaveValue } from '../value_checker'
77

8-
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods
9-
type HttpMethod =
10-
| 'GET'
11-
| 'HEAD'
12-
| 'POST'
13-
| 'PUT'
14-
| 'DELETE'
15-
| 'CONNECT'
16-
| 'OPTIONS'
17-
| 'TRACE'
18-
| 'PATCH'
8+
type HttpMethod = 'GET' | 'POST' | 'PUT'
199

2010
/**
2111
* This Writable writes data to a HTTP/HTTPS URL.
@@ -27,18 +17,18 @@ type HttpMethod =
2717
*
2818
* 3xx redirects are not currently followed.
2919
*/
30-
export default class HttpStream extends Writable {
20+
export default class HttpStream extends Transform {
3121
private tempFilePath: string
3222
private tempFile: Writable
33-
private responseBodyFromGet: string | null = null
3423

3524
constructor(
3625
private readonly url: string,
3726
private readonly method: HttpMethod,
38-
private readonly headers: { [name: string]: string },
39-
private readonly reportLocation: (content: string) => void
27+
private readonly headers: http.OutgoingHttpHeaders
4028
) {
41-
super()
29+
super({
30+
readableObjectMode: true,
31+
})
4232
}
4333

4434
_write(
@@ -49,7 +39,6 @@ export default class HttpStream extends Writable {
4939
if (this.tempFile === undefined) {
5040
tmp.file((err, name, fd) => {
5141
if (doesHaveValue(err)) return callback(err)
52-
5342
this.tempFilePath = name
5443
this.tempFile = fs.createWriteStream(name, { fd })
5544
this.tempFile.write(chunk, encoding, callback)
@@ -61,79 +50,97 @@ export default class HttpStream extends Writable {
6150

6251
_final(callback: (error?: Error | null) => void): void {
6352
this.tempFile.end(() => {
64-
this.sendRequest(
53+
this.sendHttpRequest(
6554
this.url,
6655
this.method,
67-
(err: Error | null | undefined) => {
68-
if (doesHaveValue(err)) return callback(err)
69-
this.reportLocation(this.responseBodyFromGet)
70-
callback(null)
56+
this.headers,
57+
(err1, res1) => {
58+
if (doesHaveValue(err1)) return callback(err1)
59+
this.pushResponseBody(res1, () => {
60+
this.emitErrorUnlessHttp2xx(res1, this.url, this.method)
61+
if (
62+
res1.statusCode === 202 &&
63+
res1.headers.location !== undefined
64+
) {
65+
this.sendHttpRequest(
66+
res1.headers.location,
67+
'PUT',
68+
{},
69+
(err2, res2) => {
70+
if (doesHaveValue(err2)) return callback(err2)
71+
this.emitErrorUnlessHttp2xx(res2, this.url, this.method)
72+
callback()
73+
}
74+
)
75+
} else {
76+
callback()
77+
}
78+
})
7179
}
7280
)
7381
})
7482
}
7583

76-
private sendRequest(
84+
private pushResponseBody(res: http.IncomingMessage, done: () => void): void {
85+
let body = Buffer.alloc(0)
86+
res.on('data', (chunk) => {
87+
body = Buffer.concat([body, chunk])
88+
})
89+
res.on('end', () => {
90+
this.push(body.toString('utf-8'))
91+
done()
92+
})
93+
}
94+
95+
private emitErrorUnlessHttp2xx(
96+
res: http.IncomingMessage,
97+
url: string,
98+
method: string
99+
): void {
100+
if (res.statusCode >= 300)
101+
this.emit(
102+
'error',
103+
new Error(
104+
`Unexpected http status ${res.statusCode} from ${method} ${url}`
105+
)
106+
)
107+
}
108+
109+
private sendHttpRequest(
77110
url: string,
78111
method: HttpMethod,
79-
callback: (err: Error | null | undefined, url?: string) => void
112+
headers: http.OutgoingHttpHeaders,
113+
callback: (err?: Error | null, res?: http.IncomingMessage) => void
80114
): void {
81115
const httpx = doesHaveValue(url.match(/^https:/)) ? https : http
116+
const additionalHttpHeaders: http.OutgoingHttpHeaders = {}
82117

83-
if (method === 'GET') {
84-
httpx.get(url, { headers: this.headers }, (res) => {
85-
if (res.statusCode >= 400) {
86-
return callback(
87-
new Error(`${method} ${url} returned status ${res.statusCode}`)
88-
)
89-
}
90-
91-
if (res.statusCode !== 202 || res.headers.location === undefined) {
92-
callback(null, url)
93-
} else {
94-
let body = Buffer.alloc(0)
95-
res.on('data', (chunk) => {
96-
body = Buffer.concat([body, chunk])
97-
})
98-
res.on('end', () => {
99-
this.responseBodyFromGet = body.toString('utf-8')
100-
this.sendRequest(res.headers.location, 'PUT', callback)
101-
})
102-
}
103-
})
104-
} else {
105-
const contentLength = fs.statSync(this.tempFilePath).size
106-
const req = httpx.request(url, {
107-
method,
108-
headers: {
109-
'Content-Length': contentLength,
110-
},
111-
})
118+
const upload = method === 'PUT' || method === 'POST'
119+
if (upload) {
120+
additionalHttpHeaders['Content-Length'] = fs.statSync(
121+
this.tempFilePath
122+
).size
123+
}
112124

113-
req.on('response', (res) => {
114-
if (res.statusCode >= 400) {
115-
let body = Buffer.alloc(0)
116-
res.on('data', (chunk) => {
117-
body = Buffer.concat([body, chunk])
118-
})
119-
res.on('end', () => {
120-
callback(
121-
new Error(
122-
`${method} ${url} returned status ${
123-
res.statusCode
124-
}:\n${body.toString('utf-8')}`
125-
)
126-
)
127-
})
128-
res.on('error', callback)
129-
} else {
130-
callback(null, url)
131-
}
132-
})
125+
const allHeaders = { ...headers, ...additionalHttpHeaders }
126+
const req = httpx.request(url, {
127+
method,
128+
headers: allHeaders,
129+
})
130+
req.on('error', (err) => this.emit('error', err))
131+
req.on('response', (res) => {
132+
res.on('error', (err) => this.emit('error', err))
133+
callback(null, res)
134+
})
133135

136+
if (upload) {
134137
pipeline(fs.createReadStream(this.tempFilePath), req, (err) => {
135-
if (doesHaveValue(err)) callback(err)
138+
if (doesHaveValue(err)) {
139+
this.emit('error', err)
140+
}
136141
})
142+
} else {
143+
req.end()
137144
}
138145
}
139146
}

0 commit comments

Comments
 (0)