-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
feat: add tests to django example #42
Conversation
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 spoke about adding Text-Runner to this repo a while ago and I still have a backlog item for it. Will do this after I'm done implementing the licensing tickets. It likely won't happen in this PR because this PR contains no Markdown files, and I prefer that each PR does only one thing.
Co-authored-by: Kevin Goslar <kevin.goslar@gmail.com>
Test is passing 🎉 |
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.
Sorry for the late review, this one slipped through my notification system. Very cool that you work on this! This is the first code review with me so naturally I have a lot of comments. Don't worry about that, this is totally normal. Just go through each comment and either apply it or come up with a good rationale for keeping your version. Both are equally okay with me, code reviews are intended to be helpful, not dominating.
Once you are through all comments, please move all the files that you have created in the root directory into the django-ory-cloud
directory. You are writing an automated test for that app, and tests should live in the same folder as the app they test.
I suggest moving one file at a time, then updating the references to it, then checking that things still work, then committing this. That way, if you mess something up, you can easily go back to the latest working state.
When you have time, we can pair up and go through a few things and concepts together or even do some pair programming.
And yes, I will focus on adding Text-Runner here and elsewhere very soon. I was tinkering all weekend on Text-Runner to update it! |
Thanks for your review Kevin, really appreciate the detail! Good point about moving the test into the directory. I went by how it is in the ory/docs repo (https://github.com/ory/docs/tree/master/tests) but in this repo it probably makes more sense to have it in the app dir (not sure why it is this way in /docs, maybe to make the CI easier). Another thing I am not 100% sure of is if we should run all tests for all examples in one workflow or if we should make a dedicated workflow for each example. |
The repo The repo here (ory/examples) is a monorepo. It contains many different codebases (the various example apps). Each code base (example app) is pretty independent of each other. They are in different programming languages (Go, Python, TypeScript) and are run separately from each other. Each codebase (example app) in this repo should have its own So, long story short, please move the tests for the Django example into a Good question around how we set up testing. I think your second option makes more sense, especially given the fact that each codebase uses a different technology and therefore might need a different docker image to run. We can also do your first option (test all codebases in one workflow), with the performance optimization that we only run the test of codebases that are being changed in that PR. I.e. don't run tests for codebases that don't get changed in a PR. This should keep that workflow short. |
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.
Good progress so far! Please let me know if you want me to help implementing some of the suggestions I made.
Thanks - lots of other topics this week but will try to finish it by EOW. |
1cff1a6
to
47dce3d
Compare
Makes sense! I wanna start with the next test next week. I also added you as maintainer here, but we could also add you to the ory mod team, then you have access to moderate all discussions as well. |
Everything but two points is resolved 🙏 |
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.
Great work! This is coming along nicely! The last open item from my end are the commented out tests. Do you have plans to make them run?
4b25b2e
to
c734c9a
Compare
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.
The finish line is in sight! This is ready to go after the last few comments are addressed. Really good work Vincent!
Alright added the last changes, thanks for the patience and reviews 🙏 |
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.
Very cool, ship it!
I propose we do a few examples and then see how much overlap there is between them. If it's just a few files (readme, playwright config), it might be easier to just copy-and-paste an existing example when one creates a new one. |
Add automated tests to the django example. The same template should then be usable for the other examples.
fyi @kevgo should we also add text-runner in this PR? it would check that all commands are correct right?
Related Issue or Design Document
Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the
maintainers to push the changes.
Further comments