Skip to content

Conversation

@DeleMike
Copy link
Collaborator

@DeleMike DeleMike commented May 5, 2025

Contributor checklist


Description

This PR contains tests for the AboutUtil.kt.
Added tests for the following AboutUtil functions:

  • Non-UI functions:

    • onShareScribeClick
    • onMailClick
  • Composable functions:

    • getCommunityList (covers all items and their click behavior)
    • getFeedbackAndSupportList (covers callback and intent-triggered items)
    • getLegalListItems

Note: onRateScribeClick test is pending. I don't know how to actually implement it. Or what to test...

Also Note: I removed dependencies relating to JUnit5. It was causing some conflicts when I tried to build/run the tests. I initially started writing with JUnit5 but I had to remove it because existing codebase already uses JUnit4 and I had an issue with Compose's compatibility with JUnit5

Related issue

DeleMike added 2 commits May 5, 2025 11:05
…functions

Added tests for the following AboutUtil functions:

- Non-UI functions:
  - onShareScribeClick
  - onMailClick

- Composable functions:
  - getCommunityList (covers all items and their click behavior)
  - getFeedbackAndSupportList (covers callback and intent-triggered items)
  - getLegalListItems

Note: `onRateScribeClick` test is pending.
@github-actions
Copy link

github-actions bot commented May 5, 2025

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 5, 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)

@DeleMike
Copy link
Collaborator Author

DeleMike commented May 5, 2025

Hi @angrezichatterbox 👋🏾, can you check out this PR? I have some issues writing the onRateScribeClick test function. I'm hoping you can help shed more light. However, overall, what do you think?

cc: @andrewtavis

@DeleMike
Copy link
Collaborator Author

DeleMike commented May 5, 2025

fixed the license issue :)
but I see this as a reason why the other two fail:

AboutUtilTest > testOnMailClick_sendsEmailIntent FAILED
    java.lang.RuntimeException at RoboMonitoringInstrumentation.java:102

AboutUtilTest > testGetCommunityListTriggersCallbacks FAILED
    java.lang.RuntimeException at RoboMonitoringInstrumentation.java:102

AboutUtilTest > testGetFeedbackAndSupportListTriggersCallbacks FAILED
    java.lang.RuntimeException at RoboMonitoringInstrumentation.java:102

AboutUtilTest > testOnShareScribeClick FAILED
    java.lang.RuntimeException at RoboMonitoringInstrumentation.java:102

AboutUtilTest > testGetLegalListItemsTriggersCallbacks FAILED
    java.lang.RuntimeException at RoboMonitoringInstrumentation.java:102

can you explain why this is happening? Instrumentation?

@angrezichatterbox
Copy link
Member

I will look into this PR tmrw evening.

@angrezichatterbox
Copy link
Member

Hey, We decided on using Mockk for the project as it offered more advantages to the project in the long term due to the Mockk being more adjusted to use Kotlin rather than Mockito as it is more adjusted to use Java.

Could the tests be migrated to use Mockk.

@DeleMike
Copy link
Collaborator Author

DeleMike commented May 6, 2025

Hey, We decided on using Mockk for the project as it offered more advantages to the project in the long term due to the Mockk being more adjusted to use Kotlin rather than Mockito as it is more adjusted to use Java.

Could the tests be migrated to use Mockk.

Yes, it can. I'll try to do that.


is that the reason why the teo tests are failing?

@angrezichatterbox
Copy link
Member

angrezichatterbox commented May 6, 2025

It would be better to write instrumentation tests where Roboelectric has to be necessary used. I would recommend writing instrumentation tests for some of these rather than Unit tests as these deals with interaction with the device as well.

@DeleMike
Copy link
Collaborator Author

DeleMike commented May 6, 2025

okay, I'll look into it and share my feedback if any

@DeleMike DeleMike marked this pull request as draft May 31, 2025 14:45
@DeleMike DeleMike marked this pull request as ready for review June 9, 2025 12:50
@DeleMike
Copy link
Collaborator Author

DeleMike commented Jun 9, 2025

Hi @angrezichatterbox , here is an update on the testing issue.

I have added new test cases and resolved the failing tests.

cc: @andrewtavis

@andrewtavis
Copy link
Member

Thanks for the great work, @DeleMike! @angrezichatterbox, let us know if anything further is needed here 😊

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.

Thanks for the PR @DeleMike. 2 of the tests are failing Could you look into that and also remove the Mockito dependencies. We will be using Mockk for the project as we had decided to use it during the initial discussion of tests.

Could you also look into the reports as I have detailed in #420 latest comment to see which parts have you missed.

@DeleMike
Copy link
Collaborator Author

Thank you @angrezichatterbox. For the reports, as discussed here, do you want me to paste the reports output here?

And for the tests, I tried it again and they are all passing. Can you highlight the two tests that are failing?

@andrewtavis
Copy link
Member

Ideally we'd also see failing tests in the PR jobs, @angrezichatterbox :) Is there a reason why the tests are passing?

@angrezichatterbox
Copy link
Member

Thank you @angrezichatterbox. For the reports, as discussed here, do you want me to paste the reports output here?

And for the tests, I tried it again and they are all passing. Can you highlight the two tests that are failing?

I'll get back to you on this later today.

@angrezichatterbox
Copy link
Member

Ideally we'd also see failing tests in the PR jobs, @angrezichatterbox :) Is there a reason why the tests are passing?

We haven't set it up for git actions for Instrumentation tests yet.

@DeleMike
Copy link
Collaborator Author

Thank you @angrezichatterbox. For the reports, as discussed here, do you want me to paste the reports output here?
And for the tests, I tried it again and they are all passing. Can you highlight the two tests that are failing?

I'll get back to you on this later today.

Alright, I'll be waiting for a reply 😊

@andrewtavis
Copy link
Member

andrewtavis commented Jun 11, 2025

Thanks for letting me know about the action, @angrezichatterbox! @DeleMike, can you make an issue to add the instrumentation tests to the GitHub actions? Including the command to run would be great, and we could do help wanted and good first issue for it 😊 You'd be welcome to work on it yourself if you have interest, but it might also be a great candidate for the hackathon in July :)

@DeleMike
Copy link
Collaborator Author

Thanks for letting me know about the action, @angrezichatterbox! @DeleMike, can you make an issue to add the instrumentation tests to the GitHub actions? Including the command to run would be great, and we could do help wanted and good first issue for it 😊 You'd be welcome to work on it yourself if you have interest, but it might also be a great candidate for the hackathon in July :)

@andrewtavis
Yes, I can make an issue. Yes, I could work on that so that we bring this PR quickly.

I will create the issue and I will tag you and @angrezichatterbox

@andrewtavis
Copy link
Member

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

@DeleMike
Copy link
Collaborator Author

Hi @andrewtavis , I will work on this PR today.

The main issue left here is, we need to wait for the Emulator to finish booting and I've been researching on how to make this possible. But I'll report my current status soon!

I will resolve merge conflicts and work on it

@DeleMike
Copy link
Collaborator Author

I have solved the merge conflicts, and the Android tests are passing on my device, but for some reason, the AndroidTests Github actions flow keeps failing. @andrewtavis @angrezichatterbox

I will investigate further.

Screenshot 2025-06-22 at 17 05 31

@andrewtavis
Copy link
Member

Nice that the tests are passing now, @DeleMike! Please let us know when this is ready for a review 😊

@DeleMike
Copy link
Collaborator Author

Hello @angrezichatterbox and @andrewtavis. The PR is ready for review.

🛠 Summary of Fixes for Failing Instrumented Tests

I resolved two core issues affecting the tests (because they were passing on physical devices and not emulators):

  1. AndroidRuntimeException – startActivity() from non-Activity context
    Issue: The ShareHelper.shareScribe() and ShareHelper.sendEmail() functions were using ContextCompat.startActivity() with a non-Activity context (e.g., Application context from InstrumentationRegistry), which throws unless FLAG_ACTIVITY_NEW_TASK is added.

Fix: I added a runtime check to append Intent.FLAG_ACTIVITY_NEW_TASK if the provided context is not an Activity:

    if (context !is Activity) {
        addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
    }
  1. NoActivityResumedException in tests like test_onShareScribeClick_doesNotCrash
  • Issue: The test directly used application context instead of launching an actual Activity.

  • Fix: I wrapped AboutUtil.onShareScribeClick() inside an ActivityScenario.launch() so that there's an active Activity:

    val scenario = ActivityScenario.launch(MainActivity::class.java)
    scenario.onActivity { activity ->
        AboutUtil.onShareScribeClick(activity)
    }

✅ All instrumentation tests now pass on the emulator started by our GitHub actions test and on physical devices.


Please let me know if you need anything else.

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.

@DeleMike Everything looks good to me. Thanks for getting this done. We are getting around 91% coverage for AboutUtil which is quite nice.

I had made some additional changes to unit test as those could be migrated to junit 5. Instrumental test can remain in Junit 4 as it is the only way.

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.

So great to have these tests up and running so early in the life of the application, @DeleMike! Thanks for the hard work here :) Quick note on comments: Would be good if ones that are their own line end with a period, but we don't need capitalization of the first letter or a period for inline comments. Thanks also for the review, @angrezichatterbox!

@andrewtavis andrewtavis merged commit 1bf18e1 into scribe-org:main Jun 24, 2025
6 checks passed
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.

Write tests for the functions in About Util

3 participants