Skip to content

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 🤔

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

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