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

CommonsApplication migrate to kotlin & some lint fixes #5879

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

parneet-guraya
Copy link
Contributor

From comment

What changes did you make and why?

  • Migrate application class to kotlin
  • some lint fixes

Tests performed (required)

tested betaDebug & prodDebug on Oneplus 9RT 5G , Android 14

@parneet-guraya
Copy link
Contributor Author

CI failing due to some unit test. Will fix it

@parneet-guraya parneet-guraya marked this pull request as draft October 21, 2024 01:02
@nicolas-raoul
Copy link
Member

I did a few uploads and tried most activities of the app with this branch. It works as usual. :-)

@parneet-guraya
Copy link
Contributor Author

Actually just one test failing due to lateinit instance property in application class, not being initialised while other tests do use that property and works fine :( . I'm a little rusty on testing, I will figure out what's causing this.

@parneet-guraya parneet-guraya marked this pull request as ready for review October 23, 2024 00:03
@parneet-guraya
Copy link
Contributor Author

parneet-guraya commented Oct 23, 2024

This test was causing the issue ThanksClientTest

`when`(CommonsApplication.getInstance()).thenReturn(commonsApplication)

when() started raising error that it needs a call to a method on mocked instance. Now, in this case we were calling instance on static, CommonsApplication.getInstance() was working fine when the class was in java but not when converted to kotlin. I'm guessing it's due to how to kotlin generate java impl internally such as it by default make properties final (I tried using open keyword` though).

Most likely this is library issue. Also, from my research on stackoverflow mocking statics is discouraged. Also I found out mockito mockk supports kotlin by design didn't try it though.

So, at the end I refactored the function to take application as an argument rather calling it inside.
Commit: d665188

@nicolas-raoul
Copy link
Member

Thanks! Would you mind fixing the conflict?

@parneet-guraya
Copy link
Contributor Author

Done

@parneet-guraya parneet-guraya marked this pull request as draft October 24, 2024 08:44
@parneet-guraya
Copy link
Contributor Author

Waiting for this one #5887

Signed-off-by: parneet-guraya <[email protected]>
Signed-off-by: parneet-guraya <[email protected]>
Signed-off-by: parneet-guraya <[email protected]>
Signed-off-by: parneet-guraya <[email protected]>
@parneet-guraya parneet-guraya marked this pull request as ready for review October 24, 2024 10:20
@parneet-guraya
Copy link
Contributor Author

Now that we have #5887 , this one's finally good to go. Give it a look whenever you can. I will have #5698 solution up after this.

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