fix(common): incorrect error type for XHR errors in TestRequest (#3…

…6082)

Currently the `HttpClient` always wraps errors from XHR requests, but
the underlying errors are always of type `ProgressEvent`, or don't have
a native error if the status code is just indicating failure (e.g. 404).

This behavior does not match in the `TestRequest` class provided by
`@angular/common/http/testing` where errors are considered being
of type `ErrorEvent`. This is incorrect because `ErrorEvent`s provide
information for errors in scripts or files which are evaluated. Since
the `HttpClient` never evaluates scripts/files, and also since XHR requests
clearly are documented to emit `ProgressEvent`'s, we should change the
`TestSupport` to retrieve such `ProgressEvent`'s instead of incompatible
objects of type `ErrorEvent`.

In favor of having a deprecation period, we keep supporting `ErrorEvent`
in the `TestRequest.error` signature. Eventually, we can remove this
signature in the future.

Resources:
  * https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/error_event
  * https://developer.mozilla.org/en-US/docs/Web/API/ErrorEvent
  * https://xhr.spec.whatwg.org/#event-xhr-errpr

Related to: #34748.

DEPRECATED: `TestRequest` from `@angular/common/http/testing` no longer
accepts `ErrorEvent` when simulating XHR errors. Instead instances of
`ProgressEvent` should be passed, matching with the native browser behavior.

PR Close #36082

aio/content/examples/http/src/testing/http-client.spec.ts

@@ -137,31 +137,21 @@ describe('HttpClient testing', () => {
   // #enddocregion 404
 
   // #docregion network-error
-  it('can test for network error', () => {
-    const emsg = 'simulated network error';
+  it('can test for network error', done => {
+    // Create mock ProgressEvent with type `error`, raised when something goes wrong
+    // at network level. e.g. Connection timeout, DNS error, offline, etc.
+    const mockError = new ProgressEvent('error');
 
     httpClient.get<Data[]>(testUrl).subscribe(
       data => fail('should have failed with the network error'),
       (error: HttpErrorResponse) => {
-        expect(error.error.message).toEqual(emsg, 'message');
+        expect(error.error).toBe(mockError);
+        done();
       }
     );
 
     const req = httpTestingController.expectOne(testUrl);
 
-    // Create mock ErrorEvent, raised when something goes wrong at the network level.
-    // Connection timeout, DNS error, offline, etc
-    const mockError = new ErrorEvent('Network error', {
-      message: emsg,
-      // #enddocregion network-error
-      // The rest of this is optional and not used.
-      // Just showing that you could provide this too.
-      filename: 'HeroService.ts',
-      lineno: 42,
-      colno: 21
-    // #docregion network-error
-    });
-
     // Respond with mock error
     req.error(mockError);
   });

aio/content/examples/testing/src/app/model/hero.service.spec.ts

@@ -186,28 +186,22 @@ describe('HeroesService (with mocks)', () => {
       req.flush(msg, {status: 404, statusText: 'Not Found'});
     });
 
-    it('should turn network error into user-facing error', () => {
-      const emsg = 'simulated network error';
+    it('should turn network error into user-facing error', done => {
+      // Create mock ProgressEvent with type `error`, raised when something goes wrong at
+      // the network level. Connection timeout, DNS error, offline, etc.
+      const errorEvent = new ProgressEvent('error');
 
       const updateHero: Hero = { id: 1, name: 'A' };
       heroService.updateHero(updateHero).subscribe(
         heroes => fail('expected to fail'),
-        error => expect(error.message).toContain(emsg)
+        error => {
+          expect(error).toBe(errorEvent);
+          done();
+        }
       );
 
       const req = httpTestingController.expectOne(heroService.heroesUrl);
 
-      // Create mock ErrorEvent, raised when something goes wrong at the network level.
-      // Connection timeout, DNS error, offline, etc
-      const errorEvent = new ErrorEvent('so sad', {
-        message: emsg,
-        // The rest of this is optional and not used.
-        // Just showing that you could provide this too.
-        filename: 'HeroService.ts',
-        lineno: 42,
-        colno: 21
-      });
-
       // Respond with mock error
       req.error(errorEvent);
     });

aio/content/examples/testing/src/app/model/hero.service.ts

@@ -81,10 +81,13 @@ export class HeroService {
       // TODO: send the error to remote logging infrastructure
       console.error(error); // log to console instead
 
-      const message = (error.error instanceof ErrorEvent) ?
-        error.error.message :
-       `server returned code ${error.status} with body "${error.error}"`;
+      // If a native error is caught, do not transform it. We only want to
+      // transform response errors that are not wrapped in an `Error`.
+      if (error.error instanceof Event) {
+        throw error.error;
+      }
 
+      const message = `server returned code ${error.status} with body "${error.error}"`;
       // TODO: better job of transforming error for user consumption
       throw new Error(`${operation} failed: ${message}`);
     };

aio/content/examples/testing/src/app/model/testing/http-client.spec.ts

@@ -121,29 +121,21 @@ describe('HttpClient testing', () => {
     req.flush(emsg, { status: 404, statusText: 'Not Found' });
   });
 
-  it('can test for network error', () => {
-    const emsg = 'simulated network error';
+  it('can test for network error', done => {
+    // Create mock ProgressEvent with type `error`, raised when something goes wrong at
+    // the network level. Connection timeout, DNS error, offline, etc.
+    const errorEvent = new ProgressEvent('error');
 
     httpClient.get<Data[]>(testUrl).subscribe(
       data => fail('should have failed with the network error'),
       (error: HttpErrorResponse) => {
-        expect(error.error.message).toEqual(emsg, 'message');
+        expect(error.error).toBe(errorEvent);
+        done();
       }
     );
 
     const req = httpTestingController.expectOne(testUrl);
 
-    // Create mock ErrorEvent, raised when something goes wrong at the network level.
-    // Connection timeout, DNS error, offline, etc
-    const errorEvent = new ErrorEvent('so sad', {
-      message: emsg,
-      // The rest of this is optional and not used.
-      // Just showing that you could provide this too.
-      filename: 'HeroService.ts',
-      lineno: 42,
-      colno: 21
-    });
-
     // Respond with mock error
     req.error(errorEvent);
   });

aio/content/guide/deprecations.md

@@ -38,6 +38,7 @@ v13 -> v16
 | `@angular/common`                   | [`ReflectiveInjector`](#reflectiveinjector)                                                                | <!--v8--> v11         |
 | `@angular/common`                   | [`CurrencyPipe` - `DEFAULT_CURRENCY_CODE`](api/common/CurrencyPipe#currency-code-deprecation)              | <!--v9--> v11         |
 | `@angular/common/http`              | [`XhrFactory`](api/common/http/XhrFactory)                                                                 | <!--v12--> v15        |
+| `@angular/common/http/testing`      | [`TestRequest` accepting `ErrorEvent` for error simulation](#testrequest-errorevent)                       | <!--v13--> v16        |
 | `@angular/core`                     | [`DefaultIterableDiffer`](#core)                                                                           | <!--v7--> v11         |
 | `@angular/core`                     | [`ReflectiveKey`](#core)                                                                                   | <!--v8--> v11         |
 | `@angular/core`                     | [`RenderComponentType`](#core)                                                                             | <!--v7--> v11         |
@@ -469,6 +470,26 @@ In ViewEngine, [JIT compilation](https://angular.io/guide/glossary#jit) required
 
 Important note: this deprecation doesn't affect JIT mode in Ivy (JIT remains available with Ivy, however we are exploring a possibility of deprecating it in the future. See [RFC: Exploration of use-cases for Angular JIT compilation mode](https://github.com/angular/angular/issues/43133)).
 
+{@a testrequest-errorevent}
+
+### `TestRequest` accepting `ErrorEvent`
+
+Angular provides utilities for testing `HttpClient`. The `TestRequest` class from
+`@angular/common/http/testing` mocks HTTP request objects for use with `HttpTestingController`.
+
+`TestRequest` provides an API for simulating an HTTP response with an error. In earlier versions
+of Angular, this API accepted objects of type `ErrorEvent`, which does not match the type of
+error event that browsers return natively. If you use `ErrorEvent` with `TestRequest`,
+you should switch to `ProgressEvent`.
+
+Here is an example using a `ProgressEvent`:
+
+```ts
+const mockError = new ProgressEvent('error');
+const mockRequest = httpTestingController.expectOne(..);
+
+mockRequest.error(mockError);
+```
 
 {@a deprecated-cli-flags}
 

goldens/public-api/common/http/testing/testing.md

@@ -49,13 +49,9 @@ export interface RequestMatch {
 export class TestRequest {
     constructor(request: HttpRequest<any>, observer: Observer<HttpEvent<any>>);
     get cancelled(): boolean;
-    error(error: ErrorEvent, opts?: {
-        headers?: HttpHeaders | {
-            [name: string]: string | string[];
-        };
-        status?: number;
-        statusText?: string;
-    }): void;
+    // @deprecated
+    error(error: ErrorEvent, opts?: TestRequestErrorOptions): void;
+    error(error: ProgressEvent, opts?: TestRequestErrorOptions): void;
     event(event: HttpEvent<any>): void;
     flush(body: ArrayBuffer | Blob | boolean | string | number | Object | (boolean | string | number | Object | null)[] | null, opts?: {
         headers?: HttpHeaders | {

packages/common/http/test/xhr_mock.ts

@@ -53,8 +53,8 @@ export class MockXMLHttpRequest {
   mockResponseHeaders: string = '';
 
   listeners: {
-    error?: (event: ErrorEvent) => void,
-    timeout?: (event: ErrorEvent) => void,
+    error?: (event: ProgressEvent) => void,
+    timeout?: (event: ProgressEvent) => void,
     abort?: () => void,
     load?: () => void,
     progress?: (event: ProgressEvent) => void,

packages/common/http/testing/src/request.ts

@@ -9,6 +9,16 @@
 import {HttpErrorResponse, HttpEvent, HttpHeaders, HttpRequest, HttpResponse, HttpStatusCode} from '@angular/common/http';
 import {Observer} from 'rxjs';
 
+/**
+ * Type that describes options that can be used to create an error
+ * in `TestRequest`.
+ */
+type TestRequestErrorOptions = {
+  headers?: HttpHeaders|{[name: string]: string | string[]},
+  status?: number,
+  statusText?: string,
+};
+
 /**
  * A mock requests that was received and is ready to be answered.
  *
@@ -78,12 +88,14 @@ export class TestRequest {
 
   /**
    * Resolve the request by returning an `ErrorEvent` (e.g. simulating a network failure).
+   * @deprecated Http requests never emit an `ErrorEvent`. Please specify a `ProgressEvent`.
+   */
+  error(error: ErrorEvent, opts?: TestRequestErrorOptions): void;
+  /**
+   * Resolve the request by returning an `ProgressEvent` (e.g. simulating a network failure).
    */
-  error(error: ErrorEvent, opts: {
-    headers?: HttpHeaders|{[name: string]: string | string[]},
-    status?: number,
-    statusText?: string,
-  } = {}): void {
+  error(error: ProgressEvent, opts?: TestRequestErrorOptions): void;
+  error(error: ProgressEvent|ErrorEvent, opts: TestRequestErrorOptions = {}): void {
     if (this.cancelled) {
       throw new Error(`Cannot return an error for a cancelled request.`);
     }