Skip to content
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

Merged
merged 7 commits into from
Sep 2, 2024

Conversation

vickcoo
Copy link
Contributor

@vickcoo vickcoo commented Sep 1, 2024

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

Copy link

github-actions bot commented Sep 1, 2024

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

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-iOS repo
  • The linting and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@vickcoo
Copy link
Contributor Author

vickcoo commented Sep 1, 2024

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.

@fabulouiOS-monk
Copy link
Collaborator

Sure @vickcoo, I will be in touch with you! 😄

@fabulouiOS-monk
Copy link
Collaborator

@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.

@andrewtavis andrewtavis self-requested a review September 1, 2024 17:06
@andrewtavis
Copy link
Member

Are we planning on creating a GitHub action to run these on PRs? I think that this would add a lot to this process :)

@andrewtavis
Copy link
Member

For Scribe.xcodeproj/project.pbxproj the addition of the testing file should change the this file, but I'm a bit confused where all the xcschemas are coming from 🤔

@vickcoo
Copy link
Contributor Author

vickcoo commented Sep 1, 2024

@fabulouiOS-monk thanks, I'll be careful with the project.pbxproj file. 😊

@vickcoo
Copy link
Contributor Author

vickcoo commented Sep 1, 2024

@andrewtavis those xcschemas files were automatically created when I create a test target

@fabulouiOS-monk
Copy link
Collaborator

fabulouiOS-monk commented Sep 2, 2024

Are we planning on creating a GitHub action to run these on PRs? I think that this would add a lot to this process :)

Are you referring to the xcschemas, No we don't need to. Devs can just exclude those from the commit.

@andrewtavis
Copy link
Member

Do we want to remove them from the PR and then put a * reference to ignore files like them in .gitignore?

@fabulouiOS-monk
Copy link
Collaborator

Do we want to remove them from the PR and then put a * reference to ignore files like them in .gitignore?

Yup, I think it is good to do that. Unless we really need some changes on those files.

@vickcoo
Copy link
Contributor Author

vickcoo commented Sep 2, 2024

I've reverted the xcscheme, which seems to work well for tests.

@andrewtavis
Copy link
Member

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 :)

@andrewtavis
Copy link
Member

We can also do a new issue for this, if need be! What are your two's thoughts on this?

@fabulouiOS-monk
Copy link
Collaborator

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 :)

Yup, lets find out. You can create an issue and ping me.

extension ExtensionTest {
func testDeletePriorToCursor() {
let string = "Hello"
let expectedResult = "Hel│"
Copy link
Member

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) 😇

Copy link
Contributor Author

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!

@andrewtavis
Copy link
Member

andrewtavis commented Sep 2, 2024

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 :)

@andrewtavis
Copy link
Member

I.e. I'd change testUnique_uniqueElements_withoutDuplicates to testUniqueElementsWithoutDuplicates and the like :)

Thoughts on this, @vickcoo and @fabulouiOS-monk :)

@fabulouiOS-monk
Copy link
Collaborator

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 norma, or can I rename them all with full pascal case? I'd fix the comment above as well :)

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.

@fabulouiOS-monk
Copy link
Collaborator

fabulouiOS-monk commented Sep 2, 2024

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.

@andrewtavis
Copy link
Member

Edit, above I mean camelCase, not PascalCase.

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 :)

@fabulouiOS-monk
Copy link
Collaborator

@andrewtavis, Yes you are correct, we have to follow coding language standards 😄.

@vickcoo
Copy link
Contributor Author

vickcoo commented Sep 2, 2024

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 :)

No problem, I'll modify the name to adhere swift conventions. 🚀
Thanks @andrewtavis, that's a good idea and it make sense. I also appreciate @fabulouiOS-monk ideas, that's a good thought 😄

@andrewtavis
Copy link
Member

I have the changes locally, @vickcoo :) Will send them along shortly!

Copy link
Member

@andrewtavis andrewtavis left a 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 💙

@andrewtavis andrewtavis merged commit e983806 into scribe-org:main Sep 2, 2024
1 check passed
@vickcoo vickcoo deleted the setup-test-with-extension-file branch September 5, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants