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

pkg/shelf/test/request_test.dart

Index: pkg/shelf/test/request_test.dart
diff --git a/pkg/shelf/test/request_test.dart b/pkg/shelf/test/request_test.dart
index ce592c7c28a85eeaec4f5d1be66abb2c456e16c2..9259dd22e989791bc2ac9b02f067d5b87c7df900 100644
--- a/pkg/shelf/test/request_test.dart
+++ b/pkg/shelf/test/request_test.dart
@@ -5,29 +5,101 @@
 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);
     });
   });
 
@@ -64,23 +136,6 @@ void main() {
           ..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", () {
@@ -105,46 +160,4 @@ void main() {
       });
     });
   });
-
-  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));
-    });
-  });
 }