Regression when $.ajax() XHR includes headers

[fatso83]

From @scottohara on May 31, 2018 7:10

Describe the bug
In sinon@5.0.7, using jQuery's $.ajax() to fetch a non-existent URL from fakeServer returns a 404 Not Found response, as expected.

In sinon@5.0.8 and later, the same fetch now returns a 0 status.

To Reproduce

1. npm install sinon@5.0.7, and run the tests below
2. npm install sinon@5.0.8, and re-run the tests again

describe.only("404 Not Found", () => {
  let fakeServer;

  beforeEach(() => (fakeServer = sinon.fakeServer.create({respondImmediately: true})));

  it("should work without headers", done => {
    $.ajax({
      url: "/bad",
      headers: {},
      error(response) {
        response.status.should.equal(404);
        response.statusText.should.equal("Not Found");
        done();
      }
    });
  });

  it("should work with headers", done => {
    $.ajax({
      url: "/bad",
      headers: {"X-FOO": 1},
      error(response) {
        response.status.should.equal(404);
        response.statusText.should.equal("Not Found");
        done();
      }
    });
  });

  afterEach(() => fakeServer.restore());
});

Expected behavior
The specs above create a fakeServer and then attempt to fetch a non-existant URL (/bad).
The only difference between the two specs is the second includes a request header, X-FOO: 1.
The specs assert the response returned from fakeServer is a 404 Not Found

In sinon@5.0.7, both specs pass.
In sinon@5.0.8 and later, the spec with the X-FOO request header fails

Screenshots
Results from sinon@5.0.7:

Results from sinon@5.0.8:

Context (please complete the following information):

• Library version: 5.0.8+
• Environment:
• Example URL:
• Other libraries you are using: jquery@3.3.1, webpack@4.10.2, chai@4.1.2, mocha@5.2.0

Copied from original issue: sinonjs/sinon#1817

[fatso83]

Probably happened in 082f8e8ab3f8b5d6332550dd05c5b3c6fed82d91, going from nise@1.2.0 to nise@1.3.3. Moving this issue over to the nise subproject.

[fatso83]

@scottohara, if you get a chance to do a git bisect to track down which commit introduced this in nise it would be very helpful.

[scottohara]

Thanks @fatso83.

git bisect shows the error is in commit d0032f7

This is the only change between v1.3.2 and v1.3.3.

Further analysis shows that the error only occurs if the value of the header being set is numeric

It looks like there's already a PR with a fix for this very issue (#48); so presumably all that needs to happen is to merge that PR and cut a new v1.3.4 release?

[fatso83]

Great find! Sometimes PRs slip throug the cracks :-) Thank you!

[fatso83]

I closed #48 because it's incorrect to supply a non-string value when setting a header value. That means supplying headers: {"X-FOO": 1} is invalid and should (?) throw an error. The alternative would be to either ignore the error and set the value to "" (empty string) or stringify the value. The latter can give unexpected results, so I'd rather be strict about this. I am thinking we should "fix" this by just giving a better error message. Test libs shouldn't be slack.

[fatso83]

So, it's not the case that supplying headers is wrong, it's supplying invalid values that fails. In that respect, this isn't a bug, but we should aim to improve the feedback.

[fatso83]

See PR #53