-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
[4/?] Add ability to run code examples in the playground: Get pony snippets tested with ponyc at CI time #550
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for pony-tutorial ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
fyi: The new job fails because currently only one code snippet has expectations set (all of them have to) |
response=$(curl -s --header "Content-Type: application/json" \ | ||
--request POST \ | ||
--data "$json" \ | ||
"https://playground.ponylang.io/evaluate.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than calling out to the playground to run these, we should use ponyc
directly here. We don't want to flood the playground when running these tests, and we don't want the uptime of the playground to affect these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I implemented it in a way I'm familiar, since nobody replied to "I still need to figure out, how that might work 🤔 If there's someone else who would take this on, I wouldn't be mad 😅" in #340. I can try doing it via ponyc
but how it will turn out is up to be shown.
}, | ||
"appendices-examples-access-command-line-arguments.pony": { | ||
"runnable": true, | ||
"stdout": "This is printed first\nThis is printed last\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to test stdout
of running the examples - we just need to prove that an example successfully compiles (or fails to compile with a particular message in stderr
). We don't need to run the examples at all - just compile them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of a snippet if it does something but we don't even know if it illustrates what the chapter talks about? It just happens to have a few buzzwords in it. Seeing them compile successfully is nice but very surface-level to me. There actually have been examples that failed running in the playground despite it successfully running was to be expected because nobody bothered to check it in the playground before. Nice letting users of the tutorial twist in the wind with compilable but unrunnable code—yay! 🎉
For things that are "ci support scripts" our pattern is to put in .ci-scripts at the top level. If we have an external script like you currently have, it should go there, not in workflows. |
When we add this, we should have a job that is triggered by the new release of a nightly image and new release of a release image to verify we dont have breakage. We have examples of this in a variety of library files. Once this basic on PR change is in, someone ping me and i'll add the "on update" workflows. |
@SeanTAllen As explained above (#550 (comment)) to @jemc, the focus of this PR is not my strong suit and it would be great if someone else could tackle this After all, I guess, [4/?] should start from scratch with a new PR as the implementation will vastly differ from what I tried. |
Fix #549
See #340