Test examples/ping-*/README.md #157
Conversation
Fails with the following output:
$ tcurl pingchannel -P hosts.json /ping '{"key": "my_key"}'
- {"ok":true,"head":null,"body":{"message":"Hello, world!","from":"127.0.0.1:300?","p":""},"headers":{"as":"json"},"trace":"*"} (glob)
+ {"ok":true,"head":null,"body":{"message":"Hello, world!","from":"127.0.0.1:3001","pheader":""},"headers":{"as":"json"},"trace":"dfabd0ded0c75adc"}
Fails with the following output:
$ tcurl pingchannel -P hosts.json /ping '{"key": "my_key"}' --headers '{"p": "my_header"}'
- {"ok":true,"head":{},"body":{"message":"Hello, world!","from_":"127.0.0.1:300?","pheader":"my_header"},"headers":{"as":"thrift"},"trace":"*"} (glob)
+ TchannelBadRequestError: no handler for service "pingchannel" and method "/ping"
+ {"ok":false,"name":"TchannelBadRequestError","message":"no handler for service \"pingchannel\" and method \"/ping\"","isError":true,"isErrorFrame":true,"errorCode":6,"type":"tchannel.bad-request","fullType":"tchannel.bad-request"}
Fails with the following output:
$ tcurl pingchannel -P hosts.json /ping '{"key": "my_key"}' --headers '{"p": "my_header"}'
- {"ok":true,"head":{},"body":{"message":"Hello, world!","from_":"127.0.0.1:300?","pheader":"my_header"},"headers":{"as":"thrift"},"trace":"*"} (glob)
+ TchannelBadRequestError: no handler for service "pingchannel" and method "/ping"
+ {"ok":false,"name":"TchannelBadRequestError","message":"no handler for service \"pingchannel\" and method \"/ping\"","isError":true,"isErrorFrame":true,"errorCode":6,"type":"tchannel.bad-request","fullType":"tchannel.bad-request"}
| @@ -3,6 +3,12 @@ go: | |||
| - 1.6 | |||
| - 1.5 | |||
|
|
|||
| sudo: false | |||
There was a problem hiding this comment.
Why is sudo: false required? IIRC, this is the default
There was a problem hiding this comment.
Leftovers from development process. Good catch, removed.
63f7f04 to
589a102
Compare
This is the default
589a102 to
32aed65
Compare
|
LGTM, happy to merge, though I do have this philosophical thought: Is storing the tests in the README actually a good idea? I know it seems cool because it's less files or whatever, but it is sane/obvious/intuitive? Note we have sacrificed formatting in the README to do this: I don't feel strongly enough about this to block the merge of this PR. Practically, I don't think this will cause us a huge issue/effort in the future. But I do think tests in the README is a bit strange and may increase the average WTFs/min in the codebase. |
|
@dansimau thanks for the comment, and this is indeed a good philosophical discussion. If we look at what we want to achieve:
I see two approaches:
(1) is a standard practice. However, since github does not really allow it easily (I want the README to be readable as-is). |
|
cc @severb can you check out the examples? |
| set -euo pipefail | ||
|
|
||
|
|
||
| declare cram_opts="-v --shell=/bin/bash" |
There was a problem hiding this comment.
I don't like the formatting of the README.md files now. Due to only two spaces they are hard to read on github. if we add --indent=4 to the cram options we can indent the code blocks by 4 spaces which will put a box around them in markdown making it a bit easier to read IMHO.
4 spaces looks much better on github
|
LGTM |
|
@dansimau we actually fixed the formatting in README. Can you re-check? |
|
LGTM |
Using cram to execute and verify instructions in the following files:
examples/ping-json/README.mdexamples/ping-thrift-gen/README.mdexamples/ping-thrift/README.mdCurrently, all of the tests are failing due to incomplete instructions in the README, but opening the pull request anyway to set the path forward to fixing them.