Display error banner when Reports server returns status code ≠ 2xx (#…

…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 <aurelien.reeves@smartbear.com> Co-authored-by: Aslak Hellesøy <aslak.hellesoy@smartbear.com> Co-authored-by: vincent.capicotto <vincent.capicotto@hiptest.net>
cucumber · Mar 19, 2021 · d845e32 · d845e32
1 parent 540b4b7
commit d845e32
Show file tree

Hide file tree

Showing 11 changed files with 241 additions and 128 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -14,6 +14,8 @@ jobs:
           - ubuntu-latest
           - windows-latest
         node-version: [10.x, 12.x, 14.x, 15.x]
+      fail-fast: false
+
     steps:
     - uses: actions/checkout@v2
     - name: with Node.js ${{ matrix.node-version }} on ${{ matrix.os }}

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -22,6 +22,7 @@ Please see [CONTRIBUTING.md](https://github.com/cucumber/cucumber/blob/master/CO
 ### Fixed
 
 * Fix types for hook functions so they can return e.g. `'skipped'` ([#1542](https://github.com/cucumber/cucumber-js/pull/1542))
+* Display the response of the reports server when an error is returned before failing. ([#1608](https://github.com/cucumber/cucumber-js/pull/1608))
 
 ## [7.0.0] (2020-12-21)
 

diff --git a/cucumber.js b/cucumber.js
@@ -1,9 +1,7 @@
 const feature = [
   '--require-module ts-node/register',
   '--require features/**/*.ts',
-  `--format ${
-    process.env.CI || !process.stdout.isTTY ? 'progress' : 'progress-bar'
-  }`,
+  `--format progress-bar`,
   '--format rerun:@rerun.txt',
   '--format usage:usage.txt',
   '--format message:messages.ndjson',

diff --git a/features/publish.feature b/features/publish.feature
@@ -54,7 +54,7 @@ Feature: Publish reports
 
   @spawn
   Scenario: Report is published when CUCUMBER_PUBLISH_TOKEN is set
-    When I run cucumber-js with arguments `` and env `CUCUMBER_PUBLISH_TOKEN=keyboardcat`
+    When I run cucumber-js with arguments `` and env `CUCUMBER_PUBLISH_TOKEN=f318d9ec-5a3d-4727-adec-bd7b69e2edd3`
     Then it passes
     And the server should receive the following message types:
       | meta             |
@@ -69,7 +69,7 @@ Feature: Publish reports
       | testStepFinished |
       | testCaseFinished |
       | testRunFinished  |
-    And the server should receive an "Authorization" header with value "Bearer keyboardcat"
+    And the server should receive an "Authorization" header with value "Bearer f318d9ec-5a3d-4727-adec-bd7b69e2edd3"
 
   @spawn
   Scenario: a banner is displayed after publication
@@ -101,6 +101,20 @@ Feature: Publish reports
       │ module.exports = { default: '--publish-quiet' }                          │
       └──────────────────────────────────────────────────────────────────────────┘
       """
+
+  @spawn
+  Scenario: when results are not published due to an error raised by the server, the banner is displayed
+    When I run cucumber-js with env `CUCUMBER_PUBLISH_TOKEN=keyboardcat`
+    Then it fails
+    And the error output contains the text:
+      """
+      ┌─────────────────────┐
+      │ Error invalid token │
+      └─────────────────────┘
+
+      Unexpected http status 401 from GET http://localhost:9987
+      """
+
   @spawn
   Scenario: the publication banner is not shown when publication is done
     When I run cucumber-js with arguments `<args>` and env `<env>`
@@ -110,10 +124,10 @@ Feature: Publish reports
       """
 
   Examples:
-    | args      | env                           |
-    | --publish |                               |
-    |           | CUCUMBER_PUBLISH_ENABLED=true |
-    |           | CUCUMBER_PUBLISH_TOKEN=123456 |
+    | args      | env                                                         |
+    | --publish |                                                             |
+    |           | CUCUMBER_PUBLISH_ENABLED=true                               |
+    |           | CUCUMBER_PUBLISH_TOKEN=f318d9ec-5a3d-4727-adec-bd7b69e2edd3 |
 
   @spawn
   Scenario: the publication banner is not shown when publication is disabled

diff --git a/features/step_definitions/cli_steps.ts b/features/step_definitions/cli_steps.ts
@@ -37,6 +37,15 @@ When(
   }
 )
 
+When(
+  /^I run cucumber-js with env `(|.+)`$/,
+  { timeout: 10000 },
+  async function (this: World, envString: string) {
+    const env = this.parseEnvString(envString)
+    return await this.run(this.localExecutablePath, [], env)
+  }
+)
+
 When(
   /^I run cucumber-js with all formatters(?: and `(|.+)`)?$/,
   { timeout: 10000 },
@@ -67,10 +76,14 @@ When(
 Then(/^it passes$/, () => {}) // eslint-disable-line @typescript-eslint/no-empty-function
 
 Then(/^it fails$/, function (this: World) {
-  const actualCode = doesHaveValue(this.lastRun.error)
+  const actualCode: number = doesHaveValue(this.lastRun.error)
     ? this.lastRun.error.code
     : 0
-  expect(actualCode).not.to.eql(0)
+
+  expect(actualCode).not.to.eql(
+    0,
+    `Expected non-zero exit status, but got ${actualCode}`
+  )
   this.verifiedLastRunError = true
 })
 

diff --git a/features/support/hooks.ts b/features/support/hooks.ts
@@ -94,7 +94,11 @@ Before('@global-install', function (this: World) {
   )
 })
 
-After(function (this: World) {
+After(async function (this: World) {
+  if (this.reportServer?.started) {
+    await this.reportServer.stop()
+  }
+
   if (
     doesHaveValue(this.lastRun) &&
     doesHaveValue(this.lastRun.error) &&
@@ -106,9 +110,3 @@ After(function (this: World) {
     )
   }
 })
-
-After(async function (this: World) {
-  if (this.reportServer?.started) {
-    await this.reportServer.stop()
-  }
-})
diff --git a/package.json b/package.json
@@ -279,7 +279,9 @@
   },
   "tsd": {
     "compilerOptions": {
-      "types": ["long"]
+      "types": [
+        "long"
+      ]
     }
   },
   "bin": {

diff --git a/src/cli/index.ts b/src/cli/index.ts
@@ -28,6 +28,7 @@ import GherkinStreams from '@cucumber/gherkin/dist/src/stream/GherkinStreams'
 import { ISupportCodeLibrary } from '../support_code_library_builder/types'
 import { IParsedArgvFormatOptions } from './argv_parser'
 import HttpStream from '../formatter/http_stream'
+import { Writable } from 'stream'
 
 const { incrementing, uuid } = IdGenerator
 
@@ -98,14 +99,26 @@ export default class Cli {
               headers.Authorization = `Bearer ${process.env.CUCUMBER_PUBLISH_TOKEN}`
             }
 
-            stream = new HttpStream(outputTo, 'GET', headers, (content) =>
-              console.error(content)
-            )
+            stream = new HttpStream(outputTo, 'GET', headers)
+            const readerStream = new Writable({
+              objectMode: true,
+              write: function (responseBody: string, encoding, writeCallback) {
+                console.error(responseBody)
+                writeCallback()
+              },
+            })
+            stream.pipe(readerStream)
           } else {
             const fd = await fs.open(path.resolve(this.cwd, outputTo), 'w')
             stream = fs.createWriteStream(null, { fd })
           }
         }
+
+        stream.on('error', (error) => {
+          console.error(error.message)
+          process.exit(1)
+        })
+
         const typeOptions = {
           cwd: this.cwd,
           eventBroadcaster,

diff --git a/src/formatter/http_stream.ts b/src/formatter/http_stream.ts
@@ -1,21 +1,11 @@
-import { pipeline, Writable } from 'stream'
+import { pipeline, Transform, Writable } from 'stream'
 import tmp from 'tmp'
 import fs from 'fs'
 import http from 'http'
 import https from 'https'
 import { doesHaveValue } from '../value_checker'
 
-// https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods
-type HttpMethod =
-  | 'GET'
-  | 'HEAD'
-  | 'POST'
-  | 'PUT'
-  | 'DELETE'
-  | 'CONNECT'
-  | 'OPTIONS'
-  | 'TRACE'
-  | 'PATCH'
+type HttpMethod = 'GET' | 'POST' | 'PUT'
 
 /**
  * This Writable writes data to a HTTP/HTTPS URL.
@@ -27,18 +17,18 @@ type HttpMethod =
  *
  * 3xx redirects are not currently followed.
  */
-export default class HttpStream extends Writable {
+export default class HttpStream extends Transform {
   private tempFilePath: string
   private tempFile: Writable
-  private responseBodyFromGet: string | null = null
 
   constructor(
     private readonly url: string,
     private readonly method: HttpMethod,
-    private readonly headers: { [name: string]: string },
-    private readonly reportLocation: (content: string) => void
+    private readonly headers: http.OutgoingHttpHeaders
   ) {
-    super()
+    super({
+      readableObjectMode: true,
+    })
   }
 
   _write(
@@ -49,7 +39,6 @@ export default class HttpStream extends Writable {
     if (this.tempFile === undefined) {
       tmp.file((err, name, fd) => {
         if (doesHaveValue(err)) return callback(err)
-
         this.tempFilePath = name
         this.tempFile = fs.createWriteStream(name, { fd })
         this.tempFile.write(chunk, encoding, callback)
@@ -61,79 +50,97 @@ export default class HttpStream extends Writable {
 
   _final(callback: (error?: Error | null) => void): void {
     this.tempFile.end(() => {
-      this.sendRequest(
+      this.sendHttpRequest(
         this.url,
         this.method,
-        (err: Error | null | undefined) => {
-          if (doesHaveValue(err)) return callback(err)
-          this.reportLocation(this.responseBodyFromGet)
-          callback(null)
+        this.headers,
+        (err1, res1) => {
+          if (doesHaveValue(err1)) return callback(err1)
+          this.pushResponseBody(res1, () => {
+            this.emitErrorUnlessHttp2xx(res1, this.url, this.method)
+            if (
+              res1.statusCode === 202 &&
+              res1.headers.location !== undefined
+            ) {
+              this.sendHttpRequest(
+                res1.headers.location,
+                'PUT',
+                {},
+                (err2, res2) => {
+                  if (doesHaveValue(err2)) return callback(err2)
+                  this.emitErrorUnlessHttp2xx(res2, this.url, this.method)
+                  callback()
+                }
+              )
+            } else {
+              callback()
+            }
+          })
         }
       )
     })
   }
 
-  private sendRequest(
+  private pushResponseBody(res: http.IncomingMessage, done: () => void): void {
+    let body = Buffer.alloc(0)
+    res.on('data', (chunk) => {
+      body = Buffer.concat([body, chunk])
+    })
+    res.on('end', () => {
+      this.push(body.toString('utf-8'))
+      done()
+    })
+  }
+
+  private emitErrorUnlessHttp2xx(
+    res: http.IncomingMessage,
+    url: string,
+    method: string
+  ): void {
+    if (res.statusCode >= 300)
+      this.emit(
+        'error',
+        new Error(
+          `Unexpected http status ${res.statusCode} from ${method} ${url}`
+        )
+      )
+  }
+
+  private sendHttpRequest(
     url: string,
     method: HttpMethod,
-    callback: (err: Error | null | undefined, url?: string) => void
+    headers: http.OutgoingHttpHeaders,
+    callback: (err?: Error | null, res?: http.IncomingMessage) => void
   ): void {
     const httpx = doesHaveValue(url.match(/^https:/)) ? https : http
+    const additionalHttpHeaders: http.OutgoingHttpHeaders = {}
 
-    if (method === 'GET') {
-      httpx.get(url, { headers: this.headers }, (res) => {
-        if (res.statusCode >= 400) {
-          return callback(
-            new Error(`${method} ${url} returned status ${res.statusCode}`)
-          )
-        }
-
-        if (res.statusCode !== 202 || res.headers.location === undefined) {
-          callback(null, url)
-        } else {
-          let body = Buffer.alloc(0)
-          res.on('data', (chunk) => {
-            body = Buffer.concat([body, chunk])
-          })
-          res.on('end', () => {
-            this.responseBodyFromGet = body.toString('utf-8')
-            this.sendRequest(res.headers.location, 'PUT', callback)
-          })
-        }
-      })
-    } else {
-      const contentLength = fs.statSync(this.tempFilePath).size
-      const req = httpx.request(url, {
-        method,
-        headers: {
-          'Content-Length': contentLength,
-        },
-      })
+    const upload = method === 'PUT' || method === 'POST'
+    if (upload) {
+      additionalHttpHeaders['Content-Length'] = fs.statSync(
+        this.tempFilePath
+      ).size
+    }
 
-      req.on('response', (res) => {
-        if (res.statusCode >= 400) {
-          let body = Buffer.alloc(0)
-          res.on('data', (chunk) => {
-            body = Buffer.concat([body, chunk])
-          })
-          res.on('end', () => {
-            callback(
-              new Error(
-                `${method} ${url} returned status ${
-                  res.statusCode
-                }:\n${body.toString('utf-8')}`
-              )
-            )
-          })
-          res.on('error', callback)
-        } else {
-          callback(null, url)
-        }
-      })
+    const allHeaders = { ...headers, ...additionalHttpHeaders }
+    const req = httpx.request(url, {
+      method,
+      headers: allHeaders,
+    })
+    req.on('error', (err) => this.emit('error', err))
+    req.on('response', (res) => {
+      res.on('error', (err) => this.emit('error', err))
+      callback(null, res)
+    })
 
+    if (upload) {
       pipeline(fs.createReadStream(this.tempFilePath), req, (err) => {
-        if (doesHaveValue(err)) callback(err)
+        if (doesHaveValue(err)) {
+          this.emit('error', err)
+        }
       })
+    } else {
+      req.end()
     }
   }
 }