pkg/shelf: case-insensitive headers, cleaner Request ctor, a lot more tests

pkg/shelf/test/request_test.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library shelf.request_test;
 
 import 'dart:async';
-import 'dart:convert';
 
 import 'package:shelf/shelf.dart';
 import 'package:unittest/unittest.dart';
 
+final _localhostUri = Uri.parse('http://localhost/');
+
 Request _request([Map<String, String> headers, Stream<List<int>> body]) {
   if (headers == null) headers = {};
-  return new Request("/", "", "GET", "", "1.1", Uri.parse('http://localhost/'),
-      headers, body: body);
+  return new Request("1.1", "GET", headers, _localhostUri,
+      body: body);
 }
 
 void main() {
-  group("contentLength", () {
-    test("is null without a content-length header", () {
-      var request = _request();
-      expect(request.contentLength, isNull);
+  group('constructor', () {
+    test('requestedUri must be absolute', () {
+      expect(() => new Request("1.1", 'GET', {}, Uri.parse('/path')),
+          throwsArgumentError);
     });
 
-    test("comes from the content-length header", () {
-      var request = _request({
-        'content-length': '42'
-      });
-      expect(request.contentLength, 42);
+    test('if uri is null, scriptName must be null', () {
+      expect(() => new Request("1.1", 'GET', {}, Uri.parse('/path'),
+          scriptName: '/script/name'), throwsArgumentError);
+    });
+
+    test('if scriptName is null, uri must be null', () {
+      var relativeUri = new Uri(path: '/cool/beans.html');
+      expect(() => new Request("1.1", 'GET', {}, Uri.parse('/path'),
+          uri: relativeUri), throwsArgumentError);
+    });
+
+    test('uri must be relative', () {
+      var relativeUri = Uri.parse('http://localhost/test');
+
+      expect(() => new Request("1.1", 'GET', {}, _localhostUri,
+          uri: relativeUri, scriptName: '/news'),
+          throwsArgumentError);
+
+      // NOTE: explicitly testing fragments due to Issue 18053
+      relativeUri = Uri.parse('http://localhost/test#fragment');
+
+      expect(() => new Request("1.1", 'GET', {}, _localhostUri,
+          uri: relativeUri, scriptName: '/news'),
+          throwsArgumentError);
+    });
+
+    test('uri and scriptName', () {
+      var pathInfo = '/pages/page.html?utm_source=ABC123';
+      var scriptName = '/assets/static';
+      var fullUrl = 'http://localhost$scriptName$pathInfo';
+      var request = new Request('1.1', 'GET', {}, Uri.parse(fullUrl),
+          uri: Uri.parse(pathInfo), scriptName: scriptName);

[nweiz, 2014/04/08 20:23:09]

Testing this with the same data in [requestedUrl] as is in [uri]/[scriptName]
isn't really testing much. The interesting case is which takes precedence when
they differ.

[kevmoo, 2014/04/08 21:41:13]

Done.

+
+      expect(request.scriptName, scriptName);
+      expect(request.pathInfo, '/pages/page.html');
+      expect(request.queryString, 'utm_source=ABC123');
+    });
+
+    test('minimal uri', () {
+      var pathInfo = '/';
+      var scriptName = '/assets/static';
+      var fullUrl = 'http://localhost$scriptName$pathInfo';
+      var request = new Request('1.1', 'GET', {}, Uri.parse(fullUrl),
+          uri: Uri.parse(pathInfo), scriptName: scriptName);
+
+      expect(request.scriptName, scriptName);
+      expect(request.pathInfo, '/');
+      expect(request.queryString, '');
+    });
+
+    test('bad uri', () {

[nweiz, 2014/04/08 20:23:09]

"bad" isn't very descriptive. What's bad about it? Same below.

[kevmoo, 2014/04/08 21:41:13]

Done.

+      var pathInfo = 'page';
+      var scriptName = '/assets/static';
+      var fullUrl = 'http://localhost$scriptName$pathInfo';
+
+      expect(() => new Request('1.1', 'GET', {}, Uri.parse(fullUrl),
+          uri: Uri.parse(pathInfo), scriptName: scriptName),
+          throwsArgumentError);
+    });
+
+    test('bad scriptName', () {
+      var pathInfo = '/page';
+      var scriptName = 'assets/static';
+      var fullUrl = 'http://localhost/assets/static/pages';
+
+      expect(() => new Request('1.1', 'GET', {}, Uri.parse(fullUrl),
+          uri: Uri.parse(pathInfo), scriptName: scriptName),
+          throwsArgumentError);
+
+      pathInfo = '/assets/static/page';

[nweiz, 2014/04/08 20:23:09]

If you're testing multiple possible errors, put them in separate test cases with
names that describe exactly what the errors are.

[kevmoo, 2014/04/08 21:41:13]

Done.

+      scriptName = '/';
+      fullUrl = 'http://localhost/assets/static/pages';
+
+      expect(() => new Request('1.1', 'GET', {}, Uri.parse(fullUrl),
+          uri: Uri.parse(pathInfo), scriptName: scriptName),
+          throwsArgumentError);
     });
   });
 
   group("ifModifiedSince", () {
     test("is null without an If-Modified-Since header", () {
       var request = _request();
       expect(request.ifModifiedSince, isNull);
     });
 
     test("comes from the Last-Modified header", () {
       var request = _request({
         'if-modified-since': 'Sun, 06 Nov 1994 08:49:37 GMT'
       });
       expect(request.ifModifiedSince,
           equals(DateTime.parse("1994-11-06 08:49:37z")));
     });
   });
 
   group("readAsString", () {

[nweiz, 2014/04/08 20:23:09]

I thought the reason you added message_test was so that you wouldn't have
duplicate tests like this one.

[kevmoo, 2014/04/08 21:41:13]

Request and Response have different logic for bodies, so thinks that map to
non-Stream body args I kept separate.

     test("supports a null body", () {
       var request = _request();
       expect(request.readAsString(), completion(equals("")));
     });
 
     test("supports a Stream<List<int>> body", () {
       var controller = new StreamController();
       var request = _request({}, controller.stream);
       expect(request.readAsString(), completion(equals("hello, world")));
 
       controller.add([104, 101, 108, 108, 111, 44]);
       return new Future(() {
         controller
           ..add([32, 119, 111, 114, 108, 100])
           ..close();
       });
     });
-
-    test("defaults to UTF-8", () {
-      var request = _request({}, new Stream.fromIterable([[195, 168]]));
-      expect(request.readAsString(), completion(equals("è")));
-    });
-
-    test("the content-type header overrides the default", () {
-      var request = _request({'content-type': 'text/plain; charset=iso-8859-1'},
-          new Stream.fromIterable([[195, 168]]));
-      expect(request.readAsString(), completion(equals("è")));
-    });
-
-    test("an explicit encoding overrides the content-type header", () {
-      var request = _request({'content-type': 'text/plain; charset=iso-8859-1'},
-          new Stream.fromIterable([[195, 168]]));
-      expect(request.readAsString(LATIN1), completion(equals("è")));
-    });
   });
 
   group("read", () {
     test("supports a null body", () {
       var request = _request();
       expect(request.read().toList(), completion(isEmpty));
     });
 
     test("supports a Stream<List<int>> body", () {
       var controller = new StreamController();
       var request = _request({}, controller.stream);
       expect(request.read().toList(), completion(equals([
         [104, 101, 108, 108, 111, 44],
         [32, 119, 111, 114, 108, 100]
       ])));
 
       controller.add([104, 101, 108, 108, 111, 44]);
       return new Future(() {
         controller
           ..add([32, 119, 111, 114, 108, 100])
           ..close();
       });
     });
   });
-
-  group("mimeType", () {
-    test("is null without a content-type header", () {
-      expect(_request().mimeType, isNull);
-    });
-
-    test("comes from the content-type header", () {
-      expect(_request({
-        'content-type': 'text/plain'
-      }).mimeType, equals('text/plain'));
-    });
-
-    test("doesn't include parameters", () {
-      expect(_request({
-        'content-type': 'text/plain; foo=bar; bar=baz'
-      }).mimeType, equals('text/plain'));
-    });
-  });
-
-  group("encoding", () {
-    test("is null without a content-type header", () {
-      expect(_request().encoding, isNull);
-    });
-
-    test("is null without a charset parameter", () {
-      expect(_request({
-        'content-type': 'text/plain'
-      }).encoding, isNull);
-    });
-
-    test("is null with an unrecognized charset parameter", () {
-      expect(_request({
-        'content-type': 'text/plain; charset=fblthp'
-      }).encoding, isNull);
-    });
-
-    test("comes from the content-type charset parameter", () {
-      expect(_request({
-        'content-type': 'text/plain; charset=iso-8859-1'
-      }).encoding, equals(LATIN1));
-    });
-  });
 }