willowtreeapps / assertk

assertions for kotlin inspired by assertj
MIT License
760 stars 85 forks source link

Add lint rules to detect testing framework assertion usages #491

Closed jzbrooks closed 9 months ago

jzbrooks commented 10 months ago

Adds a few lint checks for detecting assertions from different assertion libraries and from testing frameworks. These lint checks can be used for generic java projects also, they'll just need to apply the android lint gradle plugin.

Unit tests demonstrate the checks, but you can also ad-hoc test the library jar with an environment variable. https://googlesamples.github.io/android-custom-lint-rules/api-guide.html#publishingalintcheck

Here are some detections that show up from a fresh Android Studio project.

Screenshot 2023-11-05 at 7 59 17 PM

Quick fixes wouldn't be difficult to add, but that can also be done in a separate PR.

The detectors for assertj and google truth are off by default since they aren't included by default with the most popular testing frameworks. I don't think it would be too expensive to run them anyway.

Resolves #490

jzbrooks commented 10 months ago

I can run these checks locally. I imagine the machines running the checks in circleci are configured to use an older JDK. I've scanned the log output and have tried to inspect the config but am coming up short. These changes require a newer JDK (17 would do, but using the latest LTS makes sense if possible), but it isn't necessary to change the jvm target for the java artifact. This probably requires upgrading Gradle also.

A couple of other notes:

  1. It would be possible to ship these lint checks in an android archive (aar) so that people would get them more or less automatically when they depend on assertk.
  2. Publishing configuration needs to be sorted, but I'm happy to do that if y'all are interested.
evant commented 10 months ago

Hey! While I think this is useful, I worry about the maintenance burden, particularly with using lint which is known to have breaking changes. How would you feel about moving this to a separate repo that we don't maintain? Would be happy to link to it.

jzbrooks commented 10 months ago

That seems fine, although if having android projects to automatically get the checks simply by including assertk as a dependency sounds interesting/valuable it might make more sense here.

I've maintained lint checks for years and haven't encountered a single breaking change (which is not to say it will never happen, just that it hasn't happened to me in quite some time).

That said, building and maintaining lint checks can be onerous—so I get your concern.

evant commented 10 months ago

having android projects to automatically get the checks simply by including assertk as a dependency sounds interesting/valuable it might make more sense here

While I think this is a useful tool, I don't think it should be enabled by default actually, there's many ways one might have their project set up where they are purposely using multiple types of assertions.

I've maintained lint checks for years and haven't encountered a single breaking change

That's good to hear! I tried making a custom lint ages ago and it was a huge headache, things must of stabilized since then.

That said, building and maintaining lint checks can be onerous—so I get your concern.

If you check out the recent commit history on this repo it's been slow going. Wish we could spend more bandwidth on it, but that's not in the cards unfortunately, so yeah I still think this is will be better supported externally at least for the time being.

jzbrooks commented 9 months ago

I moved them to https://github.com/jzbrooks/assertk-lint. I plan to publish them at the maven coordinates com.jzbrooks:assertk-lint:<version>. We may move them into the Chick-fil-a org eventually.

evant commented 9 months ago

Awesome! Put up a pr to link to this in the README, feel free to take a look https://github.com/willowtreeapps/assertk/pull/492