-
Notifications
You must be signed in to change notification settings - Fork 488
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
✅ Unit test ios target #1168
✅ Unit test ios target #1168
Conversation
- basic ios target test - refactors existing android test for code sharing - changes `call_build_package()` to simple function Grows `buildozer/targets/ios.py` coverage from 11% to 24%. This setups the canvas for more tests to come later.
@@ -3,9 +3,6 @@ | |||
''' | |||
|
|||
import sys | |||
if sys.platform != 'darwin': | |||
raise NotImplementedError('Windows platform not yet working for Android') |
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 got the message a bit wrong, but also this got moved to check_requirements()
instead. It feels more appropriated and improves testability
def default_specfile_path(): | ||
return os.path.join(os.path.dirname(buildozer_module.__file__), "default.spec") | ||
|
||
def call_build_package(target_android): |
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.
Changed to a more "functional approach" to make it clear it only requires target_android and won't mutate self
tests/targets/test_android.py
Outdated
with patch_target_android( | ||
'_update_libraries_references' | ||
) as m_update_libraries_references, patch_target_android( | ||
'_generate_whitelist' | ||
) as m_generate_whitelist, mock.patch( | ||
'buildozer.targets.android.TargetAndroid.execute_build_package' | ||
) as m_execute_build_package, mock.patch( | ||
'buildozer.targets.android.copyfile' | ||
) as m_copyfile, mock.patch( | ||
'buildozer.targets.android.os.listdir' | ||
) as m_listdir: | ||
m_listdir.return_value = ['30.0.0-rc2'] | ||
target_android.build_package() |
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.
I really dislike this with statement formatting :<
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.
sure, let me know the way you like it so I can change it to it
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.
I've updated it to the version with the backslash https://github.com/psf/black/blob/7403d95/docs/the_black_code_style.md#how-black-wraps-lines let me know what you think
f743a6e
to
01b84aa
Compare
tests/targets/test_android.py
Outdated
buildozer_dir=buildozer.buildozer_dir) | ||
) | ||
|
||
with \ |
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.
Breaking the first line here seems a bit pointless, it actually leads to more indentation rather than less!
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.
I think it's because indentation needs to be a multiple a 4 and not under/over indented (unless the flake8 rule is commented which is the case) so if you want them aligned that's how you can address it.
I've changed it anyway, let me know what do you think
01b84aa
to
ece5748
Compare
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
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
call_build_package()
to simple functionGrows
buildozer/targets/ios.py
coverage from 11% to 24%.This setups the canvas for more tests to come later.