Skip to content

Conversation

@catreedle
Copy link
Collaborator

Contributor checklist


Description

This PR adds new file /src/test/kotlin/be/scri/ui/screens/about/AboutUtilTest.kt to test the functionality of AboutScreen also refactor the code in ShareHelper.kt and AboutUtil.kt

Related issue

@github-actions
Copy link

Thank you for the pull request! ❤️

The Scribe-Android team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Android rooms once you're in. Also consider attending our bi-weekly Saturday dev syncs. It'd be great to meet you 😊

@github-actions
Copy link

github-actions bot commented May 31, 2025

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • The linting and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis
Copy link
Member

Thanks so much for the PR here, @catreedle! We'll try to get to the review in the coming days :)

Copy link
Member

@angrezichatterbox angrezichatterbox left a comment

Choose a reason for hiding this comment

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

@catreedle Great work on the tests. This is something you could look into after which we could merge this PR.

Could you try running the command ./gradlew jacocoTestReport

We would be able to see the test report inside the Scribe-Android/app/build/reports/jacoco/html

If you open the index.html and open the click on the be.scri.about and AboutUtil you would be able to see which all parts you missed.

@andrewtavis Should we aim for 100 % percentage coverage for all the files or a certain range initially.

@catreedle
Copy link
Collaborator Author

Thank you @angrezichatterbox
will continue work on it 😊

@andrewtavis
Copy link
Member

@andrewtavis Should we aim for 100 % percentage coverage for all the files or a certain range initially.

What's the current coverage percent? I think we can have it be for the current percent and then make an issue to improve it to 100%?

@angrezichatterbox
Copy link
Member

@andrewtavis Should we aim for 100 % percentage coverage for all the files or a certain range initially.

What's the current coverage percent? I think we can have it be for the current percent and then make an issue to improve it to 100%?

It currently 40 percent

@andrewtavis
Copy link
Member

Let's definitely make an issue for it then and not worry to hit 100% for now :)

@andrewtavis
Copy link
Member

#411 has been merged in and we're ready to work on other PRs, @catreedle :) Please let us know if you need assistance with the merge conflicts here!

@catreedle
Copy link
Collaborator Author

#411 has been merged in and we're ready to work on other PRs, @catreedle :) Please let us know if you need assistance with the merge conflicts here!

hey @andrewtavis sorry I just read this. looking into it now

@catreedle
Copy link
Collaborator Author

catreedle commented Jul 4, 2025

hi @andrewtavis @DeleMike @angrezichatterbox
I've resolved the conflicts. please review and let me know if this needs additional work 😊

@andrewtavis
Copy link
Member

Thanks for getting to the conflicts, @catreedle! Would be great if we could fix the failing instrumentation tests, but I'm sure @angrezichatterbox and @DeleMike can support with that 😊

@angrezichatterbox
Copy link
Member

Hey @catreedle
These tests fail locally as well right. Could you try fixing those tests. Happy to help if you are facing difficulties :)

@catreedle
Copy link
Collaborator Author

hi @angrezichatterbox @DeleMike
I've made some small changes to AboutUtilInstrumentedTest (testGetCommunityList)
please review and let me know if it's ok or not 😊

@andrewtavis andrewtavis requested a review from DeleMike July 13, 2025 23:39
@DeleMike
Copy link
Collaborator

Hii @catreedle, thank you!

I will check it out :)

@angrezichatterbox
Copy link
Member

Hey @catreedle

It looks good to me. We could merge this in and create more issues. This could be an example for others to make further test PR to the project so it would be helpful for this to get merged.

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

All's looking great to me, @catreedle! Thanks so much for the focus on tests! Great to have such a solid team working on Scribe-Android 😊

@andrewtavis andrewtavis merged commit 72e37b3 into scribe-org:main Jul 18, 2025
6 checks passed
@andrewtavis andrewtavis mentioned this pull request Jul 18, 2025
2 tasks
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.

4 participants