Expand Mock option explanation for use with multiple issues.

utwyko commented 3 months ago

Summary

I spend quite some time figuring out why configuring the various "Do Not Mock" lint checks were not working with Mockito Kotlin, even after configuring the mock-factories through lint.xml. I configured it like this:

    <issue id="DoNotMock">
        <option name="mock-factories" value="org.mockito.kotlin.MockingKt#mock,org.mockito.kotlin.SpyingKt#spy" />
    </issue>

    <issue id="DoNotMockDataClass">
        <option name="mock-factories" value="org.mockito.kotlin.MockingKt#mock,org.mockito.kotlin.SpyingKt#spy" />
    </issue>

    <issue id="DoNotMockObjectClass">
        <option name="mock-factories" value="org.mockito.kotlin.MockingKt#mock,org.mockito.kotlin.SpyingKt#spy" />
    </issue>

However, it did not work until I added the option for DoNotMockRecordClass

    <issue id="DoNotMockRecordClass">
        <option name="mock-factories" value="org.mockito.kotlin.MockingKt#mock,org.mockito.kotlin.SpyingKt#spy" />
    </issue>

It turns out that Lint overrides the option to the value used in the latest registered issue (tested in AGP 8.3.0 and 8.4.0-beta02 and their lint checks). So setting it up just for DoNotMockRecordClass works, as well as setting it up for all issues with the option.

This PR adds this to the explanation of the option. I understand this is not an issue with this project and it might not be relevant for Slack since it's already set up correctly, so feel free to decline this. But I figured I'd put up a PR in case folks running into the same problem.

Thanks for the work on this project 🙏.

Requirements (place an `x` in each `[ ]`)

[x] I've read and understood the Contributing Guidelines and have done my best effort to follow them.
[x] I've read and agree to the Code of Conduct.

The following point can be removed after setting up CI (such as Travis) with coverage reports (such as Codecov)

[x] I've written tests to cover the new code and functionality included in this PR.

The following point can be removed after setting up a CLA reporting tool such as cla-assistant.io

[x] I've read, agree to, and signed the Contributor License Agreement (CLA).

salesforce-cla[bot] commented 3 months ago

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Wyko Rijnsburger w***@d***.com. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated.

salesforce-cla[bot] commented 3 months ago

Thanks for the contribution! Before we can merge this, we need @utwyko to sign the Salesforce Inc. Contributor License Agreement.

ZacSweers commented 3 months ago

I'm not sure I understood - are you saying that in order for one option to work in one lint, it needs to be configured for all lints that use the same option?

utwyko commented 3 months ago

Yes, that's correct. Because the RecordClassMockDetector is the last issue in the list of issues for which these options are set, the value set for that issue is applied. You could technically only set it for that issue, but then you're relying on the order of the issues, which could change at any time.

You can also see this behavior in the Lint suggestion output. When there is a violation for DoNotMockDataClass, Lint suggest to update the option for DoNotMockRecordClass.

Explanation for issues of type "DoNotMockDataClass":
   data classes represent pure data classes, so mocking them should not be
   necessary. Construct a real instance of the class instead.

   Available options:

   **mock-annotations** (default is "org.mockito.Mock,org.mockito.Spy"):
   A comma-separated list of mock annotations..

   This property should define comma-separated list of mock annotation class names (FQCN).

   To configure this option, use a `lint.xml` file with an <option> like this:

   <lint>
       <issue id="DoNotMockRecordClass">
           <option name="mock-annotations" value=""org.mockito.Mock,org.mockito.Spy"" />
       </issue>
   </lint>

I'll also try to get a ticket for this up on Google's issue tracker as I don't think Lint should behave like this.

ZacSweers commented 2 months ago

Yeah if you don't mind linking an issue with this, I'm game to merge it. That definitely seems like a bug on their end

ZacSweers commented 2 months ago

@utwyko did you file anything?

utwyko commented 1 month ago

@ZacSweers sorry for taking so long. For a reason I've still yet to figure out, I had trouble getting the DoNotMock lint rules to work in a repro project. I went a different route showcasing it with the Resource Import rules. Issue is here: https://issuetracker.google.com/issues/342545088

slackhq / slack-lints