More gradle lint

[mikehardy]

Purpose / Description

Much to my annoyance I continue to see gradle 9 deprecation warnings in Android Studio

I hunted down a couple more with generic "./gradlew test" or "./gradlew build" type targets

None of this is really important.

Fixes

Nothing logged, but I don't want unexpected build errors when gradle 9 comes out, and it should be shortly

Approach

The gradle deprecation warnings are at least very informative, they contain links to the page describing what to do

How Has This Been Tested?

These changes were basically all test-related, so, testing was pretty obviously to keep running the test targets

Learning (optional, can help others)

just handling deprecation in toolchains is a significant chunk of my life

Also, it seems like there might be some internal chunk of Android Studio that contains deprecation itself, as one final target seems to continue triggering things - amazonDebugRuntimeClasspathCopy but I can't locate that target so I dunno

We may find out when gradle 9 launches

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[mikehardy]

one spacing change, I just threw it in there. Forgive me

[david-allison]

Looks great!

Does this fix:

• CI: common module: tests don't run #16618

[Arthur-Milchior]

I admit it took me a minute to understand the expected route through the code.
I'd consider maybe,

    @Test
    fun failsWithIncorrectException() {
        var assertionErrorThrown = false
        try {
            assertThrows<IllegalArgumentException>("IllegalArgumentException is not found") {
                throw NullPointerException("this is unexpected")
            }
        } catch (e: AssertionError) {
            // we expect this
            assertThat(
                "message is expected",
                e.message?.contains("unexpected exception type thrown") == true
            )
            assertionErrorThrown = true
    }
    assertThat("Should have had an AssertionError", assertionErrorThrown)
}

This way, I don't have to think about why there is an inconditional fail at the end of the test

[mikehardy]

🤔 I think it's still cleaner without an extra local var and comparison but if it is unclear at all I think there is an opportunity to improve the message in the fail itself so it's completely clear. What if it was like

fail("only reachable if no exception or exception did match correctly - both are always a failure")

[david-allison]

If we're going to nitpick on tests

• use containsString rather than e.message?.contains("unexpected exception type thrown") == true
• extract to something like captureAssertionFailure

@Test
    fun failsWithIncorrectException() {
        val assertionError = captureAssertionFailure {
            assertThrows<IllegalArgumentException>("IllegalArgumentException is not found") {
                throw NullPointerException("wrong exception type")
            }
        }

        assertThat(assertionError.message, containsString("unexpected exception type thrown"))
    }

    @Test
    fun failsWithNoException() {
        val assertionError = captureAssertionFailure {
            assertThrows<IllegalArgumentException>("No exception is found") {
                // assertThrows should fail: nothing is thrown inside this block
            }
        }

        assertThat(assertionError.message, containsString("nothing was thrown"))
    }

    /**
     * Asserts that the provided block throws an [AssertionError]
     * @param throwErrorBlock code which should throw an [AssertionError]
     * @return the [AssertionError] thrown by [throwErrorBlock]
     */
    private fun captureAssertionFailure(throwErrorBlock: () -> Unit): AssertionError {
        try {
            throwErrorBlock()
        } catch (e: AssertionError) {
            return e
        }
        // this statement may not be inside the try ... as it would be caught by the 'catch'
        fail("An AssertionError should have been thrown")
    }

[Arthur-Milchior]

That's a weak opinion, if you prefer this version, fine by me

[david-allison]

[minor, out of personal curiosity]

@Arthur-Milchior Could I confirm the meaning of 'weak opinion' here?

Who has one, or are there readability problems with my code above?

[mikehardy]

I'm going to guess (correct if I am wrong!) that the weakness was between my take on it and Arthur's which were both the same basic implementation. Yours is a totally different thing and appears more expressive to me, I'll adopt it

[mikehardy]

Looks great!

Does this fix:

• CI: common module: tests don't run #16618

I did not test that but there is no real rush getting this in. I'll give it a look and see

[david-allison]

Let's see if it fixes the common testing issue 🤞

• https://github.com/david-allison/Anki-Android/actions/runs/11786385567

EDIT: It doesn't

[david-allison]

Adding a Needs Author Reply Waiting for a reply from the original author on the test changes (or not): https://github.com/ankidroid/Anki-Android/pull/17363/files#r1836879056

I'm ambivalent. Implementer's choice, as long as CI is green, happy to merge

[mikehardy]

I adopted the test kotlinization you proposed David
Also noticed our lint wasn't applied in testlib or common so added a comment where I apply our lint config

[mikehardy]

Added a (final?) commit that disables the custom scheduler test as Flaky in CI again. Sad to do it but it's causing a lot of issues

[david-allison]

Force pushed a very minor nit. Let's go!