-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make .md
s testable
#198
Comments
I think that's a great idea. It would probably take a bit more to include assertions but it might be worth just having CI run them as part of the normal flow |
I strongly feel the importance of this feature because interledger-rs is rapidly growing and there are many changes which affect documents. I'm thinking of what is the right approach to implement this.
This option only tests whether the result is correct or not. Although it seems correct approach as an integrated test, may be inefficient because it cannot reuse exit codes and sometimes it is hard to define what is the correct state as the result. or... should we support both? What do you think? @interledger-rs/contributors |
Rather than adding new tests within each file that are hidden from view, I would like to invoke
...we could also write each JSON response to a different file in the
I haven't taken a look at the other examples to see if they produce testable output, but if not then perhaps they could be changed to do so. |
Thank you Ben. I understand the approach. One thing I'm curious about is why you would like the approach. |
@dora-gt My biggest concern is that I want to make sure that the examples on the master branch run successfully at all times. My reason is selfish: I use the examples as an easy way to simulate real traffic between nodes and to demonstrate to myself that my commits are working as expected. For example, right now I'm adding functionality that will allow users to subscribe to a feed that will alert them when they have incoming payments, and to show that it works I run the simple example and wait for the notifications. However, over the past two weeks the simple example has been repeatedly broken (it is broken on master right now, for example) which sidetracks me into having to look into that first. I would be fine with the version that you suggested previously where we insert hidden tests into the files, but I'm afraid that those tests might not be enough to stop the examples from breaking. Can you show what sort of test you have in mind here? |
(To elaborate on my specific concern in the last comment, the last two bugs that I have fixed in the simple example would likely only have been found by integration tests such as those performed by running the examples: the first was a bug in HTTP request username deserialization that only manifested during account update, not account creation; the second was a bug in environment variable deserialization that would only be triggered by actually trying to run the interledger executable.) |
Yes, we should achieve this goal to welcome the other developers. I'm sorry that examples get broken so often. I'll solve the issue, implementing this feature. Once implemented, it will be easier to notice that the examples are not working. What I was thinking about inserting tests in
but... if the section includes multiple commands, this might not work because it might be able to use only the last exit code. It seems that testing the last state (or some output which means an integrated state) would suffice the objective so far. So let's start with the integrated test, and if we find some specific situations that evade the integrated test, then I would consider unit tests. Thank you Ben. If there are objections, please tell me. > other guys. |
Don't worry, it's not your fault and not your responsibility to test the examples against every single commit. :)
I had looked into this in the past and I believe that curl does not actually return a non-zero exit code when it receives an HTTP status code other than 200. Source: https://superuser.com/questions/590099/can-i-make-curl-fail-with-an-exitcode-different-than-0-if-the-http-status-code-i |
I wrote a brief plan for this issue. If you have any opinions, please tell me. https://docs.google.com/document/d/1L-HO-j_5yMZG7JYgssH7YBYVrkhc_33Wc2Q5Nz4glFs/edit?usp=sharing |
Signed-off-by: Taiga Nakayama <dora@dora-gt.jp> interledger#198
Signed-off-by: Taiga Nakayama <dora@dora-gt.jp> interledger#198
@dora-gt I've only just seen the Google Doc with your plan, I have a question regarding this line:
Is there some reason that non-docker mode would be less desirable than docker mode for the purpose of CI? Especially since there appear to be complications regarding running docker from within docker. |
I think the main consideration is just making sure both experiences work. That said, having one of them tested is definitely an improvement over not having either tested |
Ah I see, I was under the impression that we were only testing one mode, and that docker mode was that mode. Indeed it would be good to test both, but as a minimal implementation even having one tested would be very useful in an immediate sense for catching most of the breakage to examples. |
(Sorry for this late response) I think if we provide both normal mode (I mean running locally) and docker mode, basically both should be tested. If we dare to test only one mode (maybe it is normal mode), we should have enough reason to do that. I can't help thinking of "what do users feel if something is not running?" |
Agreed. However, if we can get one done faster than the other, we should do that in the meantime |
I think I need to wait for the decision of #399 (whether we move to GitHub actions or not). If we move to GitHub actions, these should be changed according to that. |
Can this be closed? |
Yes, I'll close. |
I noticed that we made
.md
s runnable so we could even test it.It has a pro that if the documents go outdated, we will be able to notice that.
That said, it will require more efforts, so... not now but sometime in the future.
The text was updated successfully, but these errors were encountered: