Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12220068:
Test server tweaks (Closed)
|
Created:
7 years, 10 months ago by blois Modified:
6 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdding the ability for the test server to listen on non-loopback addrs.
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=18489
Patch Set 1 : #Patch Set 2 : Adding directories to dir listing. #Patch Set 3 : Fix for dirlisting. #
Total comments: 12
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 5
Patch Set 6 : Sync to ToT #Patch Set 7 : #Messages
Total messages: 10 (0 generated)
A couple of tweaks- - Made it so the test server can serve up files to other machines, so I can run tests without having to have local enlistments. - Changed access control to allow any cross origin requests (for multi-machine configs) - Added primitive directory listing so it's easier to navigate tests.
Ping! The recent test path changes broke my ability to run a custom server to test on multiple platforms. That change was rolled back, but I'd like to get this test server into usable state for manual testing before the next set of breaks come through.
@emily: Please look at the XHR stuff. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:124: request.headers['Origin']); I'm not so familiar with cross-domain requests, but here's the way I understand it: Previously we said: - If a website loaded from '127.0.0.1:$allowedPort' want's to access content on this server, we allow it. With your change: - No matter from where the request comes from (it's stored in request.headers['Origin']): we allow access to content on this server. I'm not sure if we want this behavior: If we want to test the case where access is denied your modification breaks things. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:140: var directory = new Directory(path.toNativePath()); Since there is a 'new Directory.fromPath()' we should actually use it! https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:205: files.add('$dirpath/'); AFAIK, this results in an entry in files with forward AND backward slashes on windows! https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:210: var filename = new Path.raw(file).filename; DirectoryLister gives AFAIK full native paths. So you should use 'new Path(file)' here! https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:213: var dirname = new Path.raw(file).directoryPath.filename; Same here. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:217: '<li><a href="${request.path}$filename">$filename</a></li>'); I think you implicitly assume that '${request.path}' ends with a slash. You should make sure that there is a slash (and append one otherwise).
https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:124: request.headers['Origin']); On 2013/02/12 19:00:48, kustermann wrote: > I'm not so familiar with cross-domain requests, but here's the way I understand > it: > > Previously we said: > - If a website loaded from '127.0.0.1:$allowedPort' want's to access content > on this server, we allow it. > > With your change: > - No matter from where the request comes from (it's stored in > request.headers['Origin']): we allow access to content on this server. > > I'm not sure if we want this behavior: If we want to test the case where access > is denied your modification breaks things. > Updated to use the hostname from the origin but to allow only the allowed port. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:140: var directory = new Directory(path.toNativePath()); On 2013/02/12 19:00:48, kustermann wrote: > Since there is a 'new Directory.fromPath()' we should actually use it! Done. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:205: files.add('$dirpath/'); On 2013/02/12 19:00:48, kustermann wrote: > AFAIK, this results in an entry in files with forward AND backward slashes on > windows! Cleaned this up some- it's a bit odd as the '/' was essentially a sentinel that this was a directory and I was attempting to cram everything into a single value. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:210: var filename = new Path.raw(file).filename; On 2013/02/12 19:00:48, kustermann wrote: > DirectoryLister gives AFAIK full native paths. So you should use 'new > Path(file)' here! Done. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:213: var dirname = new Path.raw(file).directoryPath.filename; On 2013/02/12 19:00:48, kustermann wrote: > Same here. Done. https://codereview.chromium.org/12220068/diff/9001/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:217: '<li><a href="${request.path}$filename">$filename</a></li>'); On 2013/02/12 19:00:48, kustermann wrote: > I think you implicitly assume that '${request.path}' ends with a slash. You > should make sure that there is a slash (and append one otherwise). Yeah, and assuming that there is a trailing slash was making the filepath handling odd as well.
LGTM with comments. But please wait for rico's response. https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:123: var origin = new Uri(request.headers['Origin'][0]); The 'Origin' header may not be present on HttpRequests (if we use 'wget/curl/...' to get a file for example). So this could crash the program!? https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:181: var os = response.outputStream; You may want to set the content-type header to html on the response.
lgtm once Martin's comments are addressed https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:179: static void sendDirectoryListing(Directory directory, HttpRequest request, can you add a little explanatory comment on this function how people can use it https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:181: var os = response.outputStream; On 2013/02/12 21:03:44, kustermann wrote: > You may want to set the content-type header to html on the response. +1 Chrome can handle it, but I've found DumpRenderTree and IE have strange behavior when content-type is not set.
https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:123: var origin = new Uri(request.headers['Origin'][0]); On 2013/02/12 21:03:44, kustermann wrote: > The 'Origin' header may not be present on HttpRequests (if we use > 'wget/curl/...' to get a file for example). > So this could crash the program!? Updated. I wasn't aware that this server was intended to cover those uses. https://codereview.chromium.org/12220068/diff/2003/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:181: var os = response.outputStream; On 2013/02/12 22:21:24, Emily Fortuna wrote: > On 2013/02/12 21:03:44, kustermann wrote: > > You may want to set the content-type header to html on the response. > > +1 Chrome can handle it, but I've found DumpRenderTree and IE have strange > behavior when content-type is not set. Updated, though I've been using it OK on IE and iOS Safari. Hopefully not too many people attempt to browse the dirs on DRT :)
LGTM https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:191: var os = response.outputStream; could we have a more describing variable name here, os seems ambiguous https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:208: os.writeString(header); move this down in the onDone, then we have all the writing done in the same place
https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:191: var os = response.outputStream; On 2013/02/13 13:31:20, ricow1 wrote: > could we have a more describing variable name here, os seems ambiguous Removed it completely, it's old habit of mine to use os for OutputStreams and is for InputStreams. Doesn't work so well for Dart though. https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:208: os.writeString(header); On 2013/02/13 13:31:20, ricow1 wrote: > move this down in the onDone, then we have all the writing done in the same > place Done. I had been hoping that I could write them out sequentially from the DirectoryLister, but unfortunately that isn't alphabetized.
Message was sent while issue was closed.
https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... File tools/testing/dart/http_server.dart (right): https://codereview.chromium.org/12220068/diff/8002/tools/testing/dart/http_se... tools/testing/dart/http_server.dart:208: os.writeString(header); On 2013/02/13 22:27:02, blois wrote: > On 2013/02/13 13:31:20, ricow1 wrote: > > move this down in the onDone, then we have all the writing done in the same > > place > > Done. I had been hoping that I could write them out sequentially from the > DirectoryLister, but unfortunately that isn't alphabetized. Yeah, I already had a big comment asking why the beep you where doing all this extra Entries work when I saw your .sort :-) |
