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

Configures Koin on project #11

Merged
merged 2 commits into from May 21, 2018
Merged

Configures Koin on project #11

merged 2 commits into from May 21, 2018

Conversation

rafaeltoledo
Copy link
Owner

Adds a basic infrastructure to allow us to mock (if needed) or replace dependencies for tests. This will be useful when implementing real features for our app.

@coveralls
Copy link

coveralls commented May 7, 2018

Pull Request Test Coverage Report for Build 59

  • 22 of 22 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 46: 0.0%
Covered Lines: 40
Relevant Lines: 40

💛 - Coveralls

@rafaeltoledo
Copy link
Owner Author

rafaeltoledo commented May 7, 2018

I know it. Dropped from 100% of coverage. ¯\_(ツ)_/¯

@rafaeltoledo rafaeltoledo force-pushed the rt/koin-for-di branch 2 times, most recently from c0ecfed to 1b6715b Compare May 7, 2018 17:25
Copy link
Collaborator

@victorolinasc victorolinasc left a comment

Choose a reason for hiding this comment

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

Everything is quite nice! The only thing that worries me is that the TestSocialApp is not just "overriding" what we want in the test but also bootstrapping a completely different global Koin instance. I think this can be improved for the test only re-configuring what it needs.

app.initKoin(newValue)

// Act
val activity = Robolectric.setupActivity(MainActivity::class.java)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems more like an arrange as well... I know that in this case we would end up with an empty Act comment, but you are not actually doing nothing in the Activity so it should be explicit that there is no action for the assertions below.

val activity = Robolectric.setupActivity(MainActivity::class.java)

// Assert
assertThat(activity).isNotNull()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there is much value in this assertion. If it is null it will fail on the line below with a very clear NPE so I'd just skip this assertion.

import org.koin.android.ext.android.startKoin
import org.koin.dsl.module.applicationContext

class TestSocialApp : Application() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is one thing here which worries me a bit: your test application has nothing to do with your production application. If possible, I'd try to minimize this by extending SocialApp class instead. This would make initializing Koin a bit more complicated though I think it pays off. IMHO it is really important to have the execution stack as close to the production as we can.

This commit not only adds Koin, but also configures a basic
infrastructure for testing, allowing us to change behaviors as needed.
@rafaeltoledo
Copy link
Owner Author

@victorolinasc please review :)

@victorolinasc
Copy link
Collaborator

Nice! It reuses the application now. I will rest much more easily now hehehe

Way to go!

@victorolinasc victorolinasc merged commit e480cf5 into develop May 21, 2018
@rafaeltoledo rafaeltoledo deleted the rt/koin-for-di branch May 21, 2018 19:29
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.

None yet

3 participants