Tests for dart-vm httpserver tutorial code excerpts #463

chalin · 2017-12-07T20:49:54Z

Contributes to #407, covering the Tutorial (VM): Write HTTP Clients and Servers (no.12)

Some code excerpts have note markers. It was a bit tricky to deal with those in the context of code excerpts that get refreshed. I've experimented with a few approaches and settled on the use of comment markers /*1*/, /*2*/, etc. Let me know what you think.

We're doing this to minimize changes to the sample code during this first pass.

chalin · 2017-12-07T20:53:04Z

examples/httpserver/bin/hello_world_server.dart

-        ..close();
-    }
-  } catch (e) {
-    print(e.toString);


The code excerpt in the tutorial page didn't contain a try-catch statement. I've reverted the code to match the excerpt, because

Catching an exception and only printing a message is bad practice. (Note only that, but the code is wrong; it prints the value of the toString function).

There are better examples of error handling elsewhere in the tutorial.

Makes sense. We'll need to update the dart-tutorials-samples repo to match, of course. (But that's true everywhere.)

Would you prefer that we point to the files in this repo? The files themselves will contain #docregion and #docplaster tags. As for a zip of the projects, it can be relatively easy to create a zip of the sources with those tags removed. We could then host those project .zip files from dartlang.org (along with the rest of the site files).

I don't feel strongly about where we point to, as long as (1) those tags aren't visible and (2) we're careful about keeping everything in sync.

kwalrath · 2017-12-07T21:03:11Z

Looking now...

kwalrath

Great work! I want to make a second pass (for missing highlights, at least) but I'm done for the night.

While you're in there, could you fix the formatting of the command-line snippets? "Wrote data for Han Solo." is unnecessarily colorized.

kwalrath · 2017-12-07T22:33:12Z

src/_tutorials/dart-vm/httpserver.md

@@ -5,7 +5,6 @@ description: Communicate over the internet
 prevpage:
  url: /tutorials/dart-vm/cmdline
  title: "Dart-VM: Write Command-Line Apps"
-css: ["httpserver.css"]
 ---
 {% capture gh-path -%}
  https://github.com/dart-lang/dart-tutorials-samples/blob/master/httpserver


We need to update this path, no? (E.g., the link in "Example file for this section: hello_world_server.dart" points here.)

I think we had a separate repo to make it easy to download all the samples easily. Now that the sites have split, all-in-one downloads seem less useful.

I guess we should aim to remove the dart-tutorials-samples repo, eventually, but that'll require some doing. For now, we should probably update that repo so that it points to the example files here.

we should probably update that repo

Yes, I'll do that for now (after this PR is accepted), and we'll keep this URL as is (so that readers will be directed to sources w/o #docregion tags).

Yep, makes sense to keep that repo for the cleaned up version of these files.

kwalrath · 2017-12-07T22:36:39Z

src/_tutorials/dart-vm/httpserver.md

@@ -154,21 +152,22 @@ listening, and responding.

 ## Binding a server to a host and port {#binding}

-_Example for this section: hello_world_server.dart._
+_Example for this section:_
+[hello_world_server.dart.]({{gh-path}}/bin/hello_world_server.dart)

 The first line of code in `main()` uses `HttpServer.bind()` to create an


line of code -> statement

kwalrath · 2017-12-07T22:57:36Z

src/_tutorials/dart-vm/httpserver.md

 {% endprettify %}
 <div class="prettify-filename">hello_world_server.dart</div>

 The code uses `await` to call the `bind` method asynchronously.
-When the bind is successful, the new HttpServer object is assigned


Why remove this?

The name of the declared variable changed and so that line of prose required editing. I decided to just drop that sentence because IMHO: (1) The code self explanatory. (2) This is especially the case given that the paragraph immediately before the code snippet reads:

The first statement in main() uses HttpServer.bind() to create an [HttpServer][] object and bind it to a host and port.

(Otherwise, the statement is essentially describing how a variable declaration with an initializer works.)

kwalrath · 2017-12-08T00:38:21Z

src/_tutorials/dart-vm/httpserver.md

@@ -204,17 +203,17 @@ The server begins listening for HTTP requests using `await for`.
 For each request received, the highlighted code is executed for that


I don't see any code highlighting. (Should I?) Maybe say instead:

For each request received, the code sends a response ('Hello, world!').

Yes, that is better (and yes I had forgotten to add the highlighting in that snippet). I've changed it to:

For each request received, the code sends a "Hello, world!" response.

kwalrath · 2017-12-08T00:49:11Z

src/_tutorials/dart-vm/httpserver.md

+- The form's `method` attribute defines
+  the kind of request, here a `GET`. Other common
+  kinds of request include POST, PUT, and DELETE.
+- Any element within the form, like `<select>`, that has a `name` becomes


-> Any element within the form that has a name, like the <select> element, becomes...

kwalrath · 2017-12-08T02:39:24Z

src/_tutorials/dart-vm/httpserver.md

+    HttpResponse response = req.response;
+
+    if (req.method == 'POST' &&
+        [!contentType != null && /*1*/!]


Could this be shortened to something like 'application/json' == contentType?.mimeType?

I see contentType?.mimeType == 'application/json' in the new code.

From ancient code reviews (don't remember whether they were even in Dart), I remember being told to use the constant as the LHS, so you don't have to worry about invoking a method on null. Is that not a concern?

No, that isn't a problem in Dart.

Operator == is defined in Object, and hence it is defined for all types, including Null.

kwalrath · 2017-12-08T02:43:14Z

src/_tutorials/dart-vm/httpserver.md

+    HttpResponse response = req.response;
+
+    if (req.method == 'POST' &&
+        [!contentType != null && /*1*/!]


It's easy to miss the /*1*/ when there's no break between the highlighted code and the footnote number. Would it be too horrible to NOT highlight the character before the /*?

Much better, thanks!

kwalrath · 2017-12-08T02:47:14Z

src/_tutorials/dart-vm/httpserver.md

-main() async {
+File targetFile = new File('index.html');
+
+Future main() async {


I kind of hate seeing main annotated with a type. It obscures the entry point for little to no benefit, as far as I can see.

Does the analyzer complain if we leave out the type?

Yes, the analyzer will complain, specifically about the following lint rule:

lint • Type annotate public APIs at ... • type_annotate_public_apis

With the increased importance of static type annotations in Dart 2, I feel that we get more benefits from enforcing the type_annotate_public_apis lint than any gains we achieve (like more terse code) from excluding it.

Without the return type, I find it easy to quickly scan the method signature main() async { ... } and "read" main() { ... }. The return type sticks out like a warning sign, alerting the reader to the fact that main is asynchronous.

It obscures the entry point

I find that the code highlighting helps make it just as easy to locate the entry point, because main() is in black font, whereas the tokens around it are more lightly colored.

The difference between the type and the method name isn't very noticeable to me:

@filiph what do you think we should do? I don't like turning off lints. Maybe we can get a special case for "main" in the lint?

Just noting that this issue isn't gating. (We are still discussing this offline.)

kwalrath · 2017-12-08T02:52:25Z

examples/httpserver/bin/hello_world_server.dart

-        ..close();
-    }
-  } catch (e) {
-    print(e.toString);


Makes sense. We'll need to update the dart-tutorials-samples repo to match, of course. (But that's true everywhere.)

kwalrath · 2017-12-08T03:00:44Z

src/_tutorials/dart-vm/httpserver.md

+  var certificateChain =
+      Platform.script.resolve('server_chain.pem').toFilePath();
+  var serverKey = Platform.script.resolve('server_key.pem').toFilePath();
+  [!var serverContext = new SecurityContext(); /*1*/!]


This new syntax is so much easier for me to read!

I'm very glad that you like it. I juggled with a few options, but this was the simplest to deal with and using /*#*/ seems natural enough a marker for code.

chalin · 2017-12-09T19:12:56Z

fix the formatting of the command-line snippets? "Wrote data for Han Solo." is unnecessarily colorized.

Let's address this separately. I've created #466 to track this issue.

chalin · 2017-12-09T22:22:39Z

Updates completed. Staging server refreshed.

chalin · 2017-12-11T18:26:27Z

examples/httpserver/bin/hello_world_server_secure.dart

 Future main() async {
-  var certificateChain =
-      Platform.script.resolve('server_chain.pem').toFilePath();
-  var serverKey = Platform.script.resolve('server_key.pem').toFilePath();


This server script, with its use of Platform.script, doesn't lend itself well to testing. We don't use Platform.script in any other app, and it is beside the point that we are trying to illustrate here, so I dropped it.

kwalrath · 2017-12-11T22:42:04Z

src/_tutorials/dart-vm/httpserver.md

+   returns an [HttpClientResponse][] object that's assigned to the `reponse`
+   variable.
+
+5. The UTF-8 response from the server is decoded. Use a transformer defined in


I think my objection was to the passive voice. How about:

/*5*/ A decoder from the ... library converts the data...

(This rewording also avoids the word transformer, which I don't think we need/want to use here.)

kwalrath · 2017-12-11T22:45:03Z

examples/httpserver/bin/basic_writer_client.dart

-  request.write(JSON.encode(jsonData));
-  HttpClientResponse response = await request.close();
-  await for (var contents in response.transform(UTF8.decoder)) {
+      .post(InternetAddress.LOOPBACK_IP_V4.host, 4049, 'file.txt'); /*1*/


At the window width I use when reviewing (just barely 3-column), this line is cut off. The last fully visible character is the ;.

kwalrath · 2017-12-11T22:45:33Z

examples/httpserver/bin/basic_writer_client.dart

+  request.headers.contentType = ContentType.JSON; /*2*/
+  request.write(JSON.encode(jsonData)); /*3*/
+  HttpClientResponse response = await request.close(); /*4*/
+  await for (var contents in response.transform(UTF8.decoder /*5*/)) {


At my window width, I can't see anything to the right of the )).

This one is a bit trickier to fix. I've reworked the code, PTAL.

kwalrath · 2017-12-11T22:48:22Z

src/_tutorials/dart-vm/httpserver.md

-  await for (HttpRequest request in requestServer) {
-    request.response..write('Hello, world!')
-                    ..close();
+import 'dart:io';


Highlight is missing on this line.

Added back.

kwalrath · 2017-12-11T22:51:21Z

src/_tutorials/dart-vm/httpserver.md

-Use a transformer defined in the `dart:convert` library
-to convert the data into regular Dart string format.
+{:.code-notes}
+1. The `post()` method requires the host, port, and the path to the requested


I think the main reason to keep it would be to emphasize that the returned value is a HttpClientRequest, since that isn't visible in the code. (You mention that type later, but you don't explicitly tie it to this variable.)

kwalrath · 2017-12-11T22:51:53Z

src/_tutorials/dart-vm/httpserver.md

+  };
+
+  var request = await new HttpClient()
+      .[!post!](InternetAddress.LOOPBACK_IP_V4.host, 4049, 'file.txt'); [!/*1*/!]


I can't see anything after the ; in this line.

I've reworked the code, PTAL.

kwalrath · 2017-12-11T22:52:50Z

src/_tutorials/dart-vm/httpserver.md

+import 'dart:convert';
+
+Future main() async {
+  var server = await HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, 4049);


The last visible characters on this line are V4,.

kwalrath · 2017-12-11T22:53:12Z

src/_tutorials/dart-vm/httpserver.md

+    if (req.method == 'POST' &&
+        [!contentType?.mimeType == 'application/json'!] [!/*1*/!]) {
+      try {
+        var [!jsonString = await req.transform(UTF8.decoder).join();!] [!/*2*/!]


Last visible characters: ();

kwalrath · 2017-12-11T22:53:35Z

src/_tutorials/dart-vm/httpserver.md

 {% prettify dart %}
 void addCorsHeaders(HttpResponse response) {
  response.headers.add('Access-Control-Allow-Origin', '*');
-  response.headers.add(
-      'Access-Control-Allow-Methods', 'POST, OPTIONS');
+  response.headers.add('Access-Control-Allow-Methods', 'POST, OPTIONS');


line is cut off

kwalrath · 2017-12-11T23:00:47Z

src/_tutorials/dart-vm/httpserver.md

-main() async {
+File targetFile = new File('index.html');
+
+Future main() async {


The difference between the type and the method name isn't very noticeable to me:

kwalrath

A few code snippets need a shorter line length, and I'd like some wording tweaks, but once you fix those LGTM.

chalin · 2017-12-12T14:14:53Z

src/_tutorials/dart-vm/httpserver.md

+3. The data sent by the client is JSON formatted. The server decodes it using
+   the JSON codec available in the [dart:convert][] library.
+
+4. The URL for the request is


FYI, the text of these bullets has not changed. I've just swapped the steps because of a reordering of statements in the code.

Contributes to #407. Followup to #463. - Remove unnecessary pubspec dependency. - Fix marker placement.

This brings in the changes made under: - dart-lang/site-www#463 - dart-lang/site-www#470

The code was "migrated" to Dart 2 via #463, and its completion status was marked as done in #664.

chalin added 5 commits December 7, 2017 11:22

A first httpserver test for hello_world_server

f4ebb64

WIP: tests for class-based number thinker and guesser

2ec3f1c

Revert to global-function-based version of number thinker and guesser

389d5b1

We're doing this to minimize changes to the sample code during this first pass.

Re-insert docregions into number_thinker

c0ea065

Wrapup of the rest of the httpserver tutorial

80b122e

chalin added fix.examples Adds or changes example test.general Relates to unit, integration, perf testing labels Dec 7, 2017

googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Dec 7, 2017

chalin commented Dec 7, 2017

View reviewed changes

chalin changed the title ~~Chalin httpserver 1206~~ Tests for dart-vm httpserver tutorial code excerpts Dec 7, 2017

chalin requested a review from kwalrath December 7, 2017 21:01

chalin mentioned this pull request Dec 7, 2017

Code excerpts should be auto-updated and tested #407

Closed

20 tasks

kwalrath reviewed Dec 8, 2017

View reviewed changes

chalin mentioned this pull request Dec 9, 2017

Support better highlighting of command-line snippets #466

Closed

Post-review edits

fc253a9

chalin mentioned this pull request Dec 9, 2017

Update the dart-tutorials-samples repo code to match sources under examples/httpserver #467

Closed

Provide partially operational test for hello_world_server_secure

fa2366c

chalin mentioned this pull request Dec 11, 2017

Add tests for hello_world_server_secure.dart #468

Closed

chalin commented Dec 11, 2017

View reviewed changes

kwalrath reviewed Dec 11, 2017

View reviewed changes

kwalrath approved these changes Dec 11, 2017

View reviewed changes

Post-review edits, take 2

1fbefa0

chalin commented Dec 12, 2017

View reviewed changes

chalin merged commit f6bc647 into master Dec 12, 2017

chalin deleted the chalin-httpserver-1206 branch December 12, 2017 14:31

chalin added a commit that referenced this pull request Dec 12, 2017

Example(httpserver): minor cleanup

d0926e3

Contributes to #407. Followup to #463. - Remove unnecessary pubspec dependency. - Fix marker placement.

chalin mentioned this pull request Dec 12, 2017

Example(httpserver): minor cleanup #470

Merged

chalin added a commit that referenced this pull request Dec 12, 2017

Example(httpserver): minor cleanup (#470)

0af22cf

Contributes to #407. Followup to #463. - Remove unnecessary pubspec dependency. - Fix marker placement.

chalin added a commit to chalin/dart-tutorials-samples that referenced this pull request Dec 12, 2017

Add tests for httpserver, and update a few apps

6e86e49

This brings in the changes made under: - dart-lang/site-www#463 - dart-lang/site-www#470

chalin mentioned this pull request Dec 12, 2017

Add tests for httpserver, and update a few apps dart-archive/dart-tutorials-samples#45

Merged

chalin added a commit that referenced this pull request Jun 13, 2018

HTTP tutorial: remove non-Dart-2 warning

f4c2831

The code was "migrated" to Dart 2 via #463, and its completion status was marked as done in #664.

chalin mentioned this pull request Jun 13, 2018

HTTP tutorial: remove "not Dart-2" warning since it was updated a while ago #922

Merged

chalin added a commit that referenced this pull request Jun 13, 2018

HTTP tutorial: remove non-Dart-2 warning (#922)

2409b35

The code was "migrated" to Dart 2 via #463, and its completion status was marked as done in #664.

		@@ -204,17 +203,17 @@ The server begins listening for HTTP requests using `await for`.
		For each request received, the highlighted code is executed for that

Tests for dart-vm httpserver tutorial code excerpts #463

Tests for dart-vm httpserver tutorial code excerpts #463

Conversation

chalin commented Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

kwalrath Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwalrath commented Dec 7, 2017

kwalrath left a comment

Choose a reason for hiding this comment

kwalrath Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwalrath Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalin Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

chalin commented Dec 9, 2017

chalin commented Dec 9, 2017

Choose a reason for hiding this comment

kwalrath Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwalrath left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalin commented Dec 7, 2017 •

edited

Loading

kwalrath Dec 8, 2017 •

edited

Loading

kwalrath Dec 7, 2017 •

edited

Loading

kwalrath Dec 8, 2017 •

edited

Loading

chalin Dec 8, 2017 •

edited

Loading

kwalrath Dec 11, 2017 •

edited

Loading

kwalrath left a comment •

edited

Loading