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

Add unittest for module pythonforandroid.bootstrap #1872

Merged
merged 1 commit into from
Jun 22, 2019

Conversation

opacam
Copy link
Member

@opacam opacam commented Jun 18, 2019

In this unittest we will also cover all the bootstraps we have at this time, so this would increase the coverage a little more than the other tests I recently introduced.

Also should be mentioned that, in order that to not increase the time of our tox tests, I mocked a lot of functions, so this way the test is performed with less than a second and it covers above a 90% of the lines of the tested files (which I think that should be the final goal for our unittests...above an 90%)

As a side note: I introduced the bootstraps tests in here because our bootstraps (sdl2, webview...) inherits from Bootstrap

@AndreMiras
Copy link
Member

AndreMiras commented Jun 18, 2019

Awesome, I'll review it pretty soon.
I saw there's codacy being introduced, is it not a bit early? We're not even pep8 compliant.
Edit: actually I'm not sure how codacy got enabled 🤔

tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
@AndreMiras
Copy link
Member

This is some impressive testing on here, well done @opacam 👍
I was about to review GenericBootstrapTest, but I'd like your feedback on some of my comments first. Just to see if we're on the same page or if I'm completely off 😄
Thanks again for your efforts, the result is great!

AndreMiras
AndreMiras previously approved these changes Jun 20, 2019
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement thanks!

tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
tests/test_bootstrap.py Outdated Show resolved Hide resolved
AndreMiras
AndreMiras previously approved these changes Jun 21, 2019
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good thanks!
I leave it open a couple of days in case you want to discuss/address further the recent comments. Otherwise we're good for a merge, this is a nice improvement

self.assertIn(
mock.call().__enter__().write("sdk.dir=/opt/android/android-sdk"),
mock_open_bs.mock_calls,
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndreMiras,
now we test that we actually we write the expected values...this (and the lines below) replaces the read_data thing that it was wrong 😉

And thanks for reviewing this, your reviews allowed us to improve this pr a lot ❤️, now it looks much cleaner and we do more testing than the first posted version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That definitely makes more sense to me thanks! 👏

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial PR was already good, now it's a gem 💎
hanks for your time and patience ❤️

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.

2 participants