-
Notifications
You must be signed in to change notification settings - Fork 506
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
✅ Increases ios target test coverage #1171
Conversation
This is a follow-up for kivy#1160 and kivy#1168. Addresses the following: - grows `buildozer/targets/ios.py` target coverage from 24% to 56% - fixes `AttributeError` on `TargetIos` error call Next up should be improving: - code signing process (toggle off not fully integrated) - actual deployment (not yet tested) - further increase `buildozer/targets/ios.py` test coverage
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.
Nice job, thanks!!!
"""Checks the `toolchain build` command is called on the ios requirements.""" | ||
target = init_target(self.temp_dir) | ||
target.ios_deploy_dir = "/ios/deploy/dir" | ||
# fmt: off |
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.
Why we need this inline comments? Is buildozer using black's codestyle?
As a side note: I understand that you don't want to use the black's parenthesis style for the below lines, imho, it looks better than using back slashes...but it's my personal taste...:wink:
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.
Thanks for sharing.
Yes exactly it's to avoid black from formatting it (they're currently migrating from parenthesis to backslash after being convinced).
I personally don't care because I'm a pragmatic so I tend to just leave things the default. However I received a comment here (#1168 (comment)) that the parenthesis were disliked.
I think this is all just a matter of taste and opinion which is exactly the goal of black, to decide for us so we don't waste too much time talking about style and just chose a linter that does the job.
Hence until other devs are aligned together I'll leave it this way if you don't mind
tests/targets/test_ios.py
Outdated
mock.patch("buildozer.targets.ios.plistlib.readPlist") as m_readPlist, \ | ||
mock.patch("buildozer.targets.ios.plistlib.writePlist") as m_writePlist, \ |
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.
Why we use camel case for:
m_readPlist
m_writePlist
?
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.
Addressed, thanks!
For info I used it because my pattern is usually to simply preprend m_
to the method I'm patching
da32f4e
to
a9fc2fc
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.
💯
Thanks!!
This is a follow-up for #1160 and #1168.
Addresses the following:
buildozer/targets/ios.py
target coverage from 24% to 56%AttributeError
onTargetIos
error callNext up should be improving:
buildozer/targets/ios.py
test coverage