-
Notifications
You must be signed in to change notification settings - Fork 79
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
Setup test with extension file #498
Setup test with extension file #498
Conversation
Thank you for the pull request!The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and iOS rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you! Maintainer checklist
|
Hi @andrewtavis , this PR is a work in progress, I'll notify you when it's finished. @fabulouiOS-monk, I've started working on the unit tests and need your help. |
Sure @vickcoo, I will be in touch with you! 😄 |
@vickcoo, I think some unwanted Scribe.xcodeproj changes have also been pushed. Whenever you push changes regarding the above file it should only contain any new file modification (either deletion or addition of a file). In future to avoid this you can make use of VScode to revert the changes from Scribe.xcodeproj since from the terminal it is impossible to do so. @andrewtavis, Please suggest if those changes are required or @vickcoo can revert those changes. |
Are we planning on creating a GitHub action to run these on PRs? I think that this would add a lot to this process :) |
For |
@fabulouiOS-monk thanks, I'll be careful with the |
@andrewtavis those |
Are you referring to the xcschemas, No we don't need to. Devs can just exclude those from the commit. |
Do we want to remove them from the PR and then put a * reference to ignore files like them in |
Yup, I think it is good to do that. Unless we really need some changes on those files. |
I've reverted the |
Want to note again that it would be great if we figure out some kind of CI run for this where PRs will run the tests and we all can then see if the changes are breaking anything :) |
We can also do a new issue for this, if need be! What are your two's thoughts on this? |
Yup, lets find out. You can create an issue and ping me. |
extension ExtensionTest { | ||
func testDeletePriorToCursor() { | ||
let string = "Hello" | ||
let expectedResult = "Hel│" |
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.
Note that this should be Hell│
as we're just deleting once, right? (I'm fine with this being the result, btw) 😇
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.
Yes, that's deleting it once!
One thing that I'm noticing here is that we're mixing pascal case and snake case for the names of the test functions? Is this normal, or can I rename them all with full pascal case? I'd fix the comment above as well :) |
I.e. I'd change Thoughts on this, @vickcoo and @fabulouiOS-monk :) |
I suggested to Vickcoo to name it like this so that it's a little bit more readable than what this test case is expected to produce. |
Yes 'testUniqueElementsWithoutDuplicates' makes it good. What I thought is for other devs to understand easily just from one look at the test case name: "functionName_WhatWeExpect", So later on when some other devs are searching for a particular test case, it will be easier for them. |
Edit, above I mean I appreciate the idea here, @fabulouiOS-monk! Logically it makes sense, but the syntax is something we'll need to explain to people consistently and might get confusing. If it's not a Swift convention, then I hope it's ok that I switch them to full camel case. It was kind of difficult for me to read them at first glance, and we should be following the coding language's standards here :) |
@andrewtavis, Yes you are correct, we have to follow coding language standards 😄. |
No problem, I'll modify the name to adhere swift conventions. 🚀 |
I have the changes locally, @vickcoo :) Will send them along shortly! |
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.
Ok, so I'm hoping we're good here 😅 I made the directory just Tests
, and I think we can then mirror the project directory underneath like Tests/Keyboards/KeyboardsBase
and Tests/Scribe/...
.
Renamed the functions and did some minor edits to the arguments they're using. Hope the changes are ok!
Thank you both for all the hard work here and the great teamwork! Is annoying now, but the work we do here means that the app has a sustainable future. Really appreciate you two being willing to do the intangible work that's needed 💙
Contributor checklist
Description
This PR will set up unit tests, and create an initial unit test for
extension.swift
, This will ensure that everything is working correctly for the tests, and then we can proceed to create additional tests.Related issue