test: Fix/update the ajax tests to be a little more correct #6002

benlesh · 2021-02-08T01:53:30Z

Moved the spec updates to a different PR than #6001 so they can be reviewed separately. That way it's perhaps easier to tell they're doing the same thing.

benlesh · 2021-02-08T01:55:59Z

spec/observables/dom/ajax-spec.ts

@@ -298,39 +294,41 @@ describe('ajax', () => {

    MockXMLHttpRequest.mostRecent.respondWith({
      status: 200,
-      contentType: 'application/json',


contentType does nothing for this test. Here we assume that the browser is working as expected, and we deal with the responseType, which the browser sets for us. Since all of that process is mocked, switching to using responseType only in our tests.

benlesh · 2021-02-08T01:56:39Z

spec/observables/dom/ajax-spec.ts

    let error: any;
    const obj: AjaxConfig = {
      url: '/flibbertyJibbet',
      responseType: 'json',
      method: '',
    };

+    // No `response` property on the object (for older IE).
+    MockXMLHttpRequest.noResponseProp = true;


This is a new feature to the Mock that will strip the response property from the xhr to simulate an older browser that lacks that feature.

benlesh · 2021-02-08T01:57:01Z

spec/observables/dom/ajax-spec.ts

    ajax(obj).subscribe(
      () => {
-        throw 'should not next';
+        throw new Error('should not next');


Just making sure that errors thrown are recognized and processed properly by the test framework.

benlesh · 2021-02-08T01:58:15Z

spec/observables/dom/ajax-spec.ts

@@ -558,26 +555,17 @@ describe('ajax', () => {
      async: false,
    };

-    ajax(obj).subscribe(
-      (x: any) => {


Nothing in here mattered, accept the check of x.xhr.async, but it's unremarkable that it's in the subscribe next callback. So I've cleaned this a bit to make the test simpler.

benlesh · 2021-02-08T01:59:11Z

spec/observables/dom/ajax-spec.ts

-    expect(request.url).to.equal('/flibbertyJibbet');
+    expect(mockXHR.url).to.equal('/flibbertyJibbet');
+    // Open was called with async `false`.
+    expect(mockXHR.async).to.be.false;


Here's the most important part of this test. This is only set when the XHR's open method is called, and it defaults to true.

benlesh · 2021-02-08T01:59:56Z

spec/observables/dom/ajax-spec.ts

@@ -728,7 +716,6 @@ describe('ajax', () => {
    });

    it('should succeed on 204 No Content', () => {
-      const expected: null = null;


null didn't make any sense here. a real XHR would never have null in the responseText, or the response.

benlesh · 2021-02-08T02:00:17Z

spec/observables/dom/ajax-spec.ts

-        contentType: 'application/json',
-        responseText: expected,
+        responseType: '',
+        responseText: '',


this is more akin to what you'd see in a real call like this.

benlesh · 2021-02-08T02:01:40Z

spec/observables/dom/ajax-spec.ts

      let result: AjaxResponse<any>;
      let complete = false;

-      ajax.post('/flibbertyJibbet', expected).subscribe(


no idea why I chose to post a body of null here, seems weird. But this whole test was really just to check that 204 status passed okay.

benlesh · 2021-02-08T02:02:08Z

spec/observables/dom/ajax-spec.ts

          responseText: JSON.stringify({}),
        },
-        3
+        { uploadProgressTimes: 3 }


Just a change to configuration for these.

benlesh · 2021-02-08T02:02:56Z

spec/observables/dom/ajax-spec.ts

+  /**
+   * Set to `true` to test IE code paths.
+   */
+  static noResponseProp = false;


This is that new property that is used to simulate older browsers that would lack a response property on their XHR.

benlesh · 2021-02-08T02:03:14Z

spec/observables/dom/ajax-spec.ts

@@ -1140,19 +1133,23 @@ class MockXMLHttpRequest {
  }

  static clearRequest(): void {
+    MockXMLHttpRequest.noResponseProp = false;


Making sure we reset it here.

benlesh · 2021-02-08T02:04:06Z

spec/observables/dom/ajax-spec.ts

+  /**
+   * Used to test if `open` was called with `async` true or false.
+   */
+  public async: boolean = true;


Made this public so we can test it. (self nit: I should have removed the public and just let it be public, but we should probably use a lint rule for that)

benlesh · 2021-02-08T02:04:51Z

spec/observables/dom/ajax-spec.ts

@@ -1173,6 +1170,9 @@ class MockXMLHttpRequest {
  constructor() {
    MockXMLHttpRequest.recentRequest = this;
    MockXMLHttpRequest.requests.push(this);
+    if (MockXMLHttpRequest.noResponseProp) {
+      delete this['response'];
+    }


Above here is part of the magic for removing the response property from the XHR to simulate older browsers.

benlesh · 2021-02-08T02:05:24Z

spec/observables/dom/ajax-spec.ts

+      total?: number;
+      loaded?: number;
+    },
+    config?: { uploadProgressTimes?: number; downloadProgressTimes?: number }


New configuration options for triggering the upload and download progress events (including loadstart for each)

benlesh · 2021-02-08T02:07:08Z

spec/observables/dom/ajax-spec.ts

    this.status = response.status || 200;
    this.responseText = response.responseText;
-    const responseType = response.responseType !== undefined ? response.responseType : this.responseType;
-    if (!('response' in response)) {


I could have left this I suppose, since I'm removing the property, but I think this in check was kind of confusing, so I opted to just let this switch process. And remove the response property again after.

benlesh · 2021-02-08T02:07:24Z

spec/observables/dom/ajax-spec.ts

-    if (!('response' in response)) {
-      switch (responseType) {
-        case 'json':
-          this.jsonResponseValue(response);


this got inlined.

benlesh · 2021-02-08T02:08:12Z

spec/observables/dom/ajax-spec.ts

+      delete this['response'];
+    }
+
+    this.triggerEvent('load', { type: 'load', total: response.total ?? 0, loaded: response.loaded ?? 0 });


Added a duck-typed load event here.

test: Fix/update the ajax tests to be a little more correct

ee7fa7a

benlesh mentioned this pull request Feb 8, 2021

feat(ajax): Add option for streaming progress #6001

Merged

benlesh commented Feb 8, 2021

View reviewed changes

benlesh requested review from cartant, kolodny, kwonoj and niklas-wortmann February 8, 2021 04:16

cartant approved these changes Feb 8, 2021

View reviewed changes

benlesh merged commit 412d1fd into ReactiveX:master Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Fix/update the ajax tests to be a little more correct #6002

test: Fix/update the ajax tests to be a little more correct #6002

benlesh commented Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

benlesh Feb 8, 2021

test: Fix/update the ajax tests to be a little more correct #6002

test: Fix/update the ajax tests to be a little more correct #6002

Conversation

benlesh commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment