Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for simulating empty body response #182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fraziern
Copy link

@fraziern fraziern commented Oct 8, 2016

For issue #123 - it appears that null and undefined body responses are already handled OK by pretender. I've added tests for this, and an extra line in the README per the issue comments.

@trek
Copy link
Member

trek commented Oct 11, 2016

@fraziern can you verify that this is what actually happens? My understanding was that XHR responses for no bodies had null (or maybe undefined) response texts, not empty strings?

@fraziern
Copy link
Author

@trek good question. I did some testing and it looks like request.response is undefined, while request.responseText === ''. So adding strictEqual(request.response, undefined); and strictEqual(request.responseText, ''); to the tests doesn't break them.

If this is causing issues elsewhere, I can take a look, or I can just add the request.response check to the tests.

@trek
Copy link
Member

trek commented Oct 15, 2016

Weird, I just did a small test:

var express = require('express');
var app = express();

app.get('/', function(req, res){
  res.send('<html></html>')
});

app.get('/notfound', function(req, res){
  console.log("1")
  res.status(404).end();
});

app.listen(3001);

/*
  httpRequest = new XMLHttpRequest();
  httpRequest.open('GET', 'http://localhost:3000/404', true);
  httpRequest.send(null);
  httpRequest.onreadystatechange = function(){
    if (httpRequest.readyState === XMLHttpRequest.DONE) {
      debugger;
    }
  }

*/

and both httpRequest.response and httpRequest.responseText are "". What browser did you test in?

@fraziern
Copy link
Author

fraziern commented Oct 16, 2016

I was testing with the existing test framework, rather than a separate setup.

I was able to replicate your results in Chrome. However, if I change the responseType to json first, it returns similar results to what I got using only QUnit asserts (i.e. response is null, responseText is '').

Server:

// app.js
var express = require('express');
var app = express();
var path = require('path');

app.get('/', function(req, res) {
    res.sendFile(path.join(__dirname + '/index.html'));
});


app.get('/save', function(req, res){
  console.log("1");
  res.type('json').status(200).send().end();
});

app.listen(3000, function () {
  console.log('Example app listening on port 3000!');
});

Client:

// index.html
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title></title>
  </head>
  <body>
Testing 1, 2, 3...
  </body>

  <script type="text/javascript">
  xhr = new XMLHttpRequest();
  xhr.responseType = 'json';  // *** ADDED ***
  xhr.open('GET', 'http://localhost:3000/save', true);
  xhr.send(null);
  xhr.onreadystatechange = function(){
  if (xhr.readyState === XMLHttpRequest.DONE) {
    debugger;
  }
}
  </script>
</html>

Taking a look at the XMLHttpRequest spec, I think this is the correct result:

The response attribute must return the result of running these steps:

If responseType is the empty string or "text"
If state is not loading or done, return the empty string.

Return the text response.

Otherwise
If state is not done, return null.

If responseType is "arraybuffer"
Return the arraybuffer response.

If responseType is "blob"
Return the blob response.

If responseType is "document"
Return the document response.

If responseType is "json"
Return the JSON response.

So I think where that leaves us, is we'd want pretender to properly mimic this behavior, yes? I'm taking a look at this further.

@stefanpenner
Copy link
Contributor

so I think where that leaves us, is we'd want pretender to properly mimic this behavior, yes?

ya, we should mimic spec behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants