Introduce `assertEquals` for `int`

[Arthur-Milchior]

Currently assertEquals only consider Long and Object. In Java, Int were converted to Long, but in Kotlin they are converted to Object. Which force conversion to explicitly write toLong to get the proper overloaded method to be invoked.

I suggest adding in AnkiDroid/src/test/java/com/ichi2/testutils/AnkiAssert.java the following methods

    public static void assertEquals(int left, int right) {
        org.junit.Assert.assertEquals((long) left, (long) right);
    }

    public static void assertEquals(String message, int left, int right) {
        org.junit.Assert.assertEquals(message, (long) left, (long) right);
    }

and going through the 121 results of the search of assertEquals\(.*long in the codebase to uses this new method instead of the previous, and simplify the equality assertion slightly

[david-allison]

I don't believe this is necessary when we move to Kotlin

[Arthur-Milchior]

Can you please be more explicit? Because right now this is a problem that appeared during Kotlin migration specifically. Where the rule used to decide where overloaded method to use are distinct than the one in java.

I can imagine Kotlin provides a different solution to it, e.g. another test library for example, I'd love to know what you have in mind here

[david-allison]

Int can be cast to object. I don't see why this is necessary.

        val one: Int = 1
        val two: Int = 2
        Assert.assertEquals(one, two)

[Arthur-Milchior]

Take Assert.assertEquals(optionsGroup.visibility.toLong(), View.VISIBLE.toLong()) for example, a real line in our codebase. It was introduced during a code migration. In Java it used to be assertEquals(optionsGroup.getVisibility(), View.VISIBLE);. In this case, I strongly prefer Java version.

The reason why the translation tool added toLong was because that was implicitly used in Java already. However, Kotlin requires it to be explicit, because otherwise it would cast Int to Object instead of long.

In my opinion, tests are already hard to read. I prefer to keep test as readable as possible. After all, the goal of moving to Kotlin seems to be simplicity; adding noise is the opposite of our end goal.

So there are two solutions.

• Indeed, we could uses assertEquals on Objects. That means that the conversion would change this function call, but in a way that should not matter (and if matters, there is a huge problem that we may actually be happy to discover).
• uses assertEquals(int, int) that just convert to long and call assertEquals. I had a preference for this because this is essentially the same code as the one we had in java, and really ensured no change in the behavior.

I'm fine with both solutions. Both seems huge improvement overt the current status quo. However, it is at least important to note that what you suggest is not what we are doing today. And if we were doing what you suggest, I would not have opened this issue.

Intuitively, I'd imagine that assertEquals(int, int) is faster. Especially if we copy paste assertEquals(long, long) code instead of delegating. Because past experience with profiling showed that encapsulating integral values in objects is costly. But I must admit that unless it's actually measured, I am far from certain that the difference would in time would be noticeable in any way. The only time I really saw a cost for encapsulating was in long loop over data sent to the database during database check

[david-allison]

Shouldn't we change it to Assert.assertEquals(optionsGroup.visibility, View.VISIBLE) rather than introduce new methods?

[rohanshankar]

I can work on this if it is still an issue

[rishav0511]

@Arthur-Milchior I would love to work on this, please assign me this.

[github-actions[bot]]

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

[Arthur-Milchior]

@david-allison nope, Kotlin don't solve it. I've just tested. In the line https://github.com/ankidroid/Anki-Android/blob/main/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt#L160

if you remove the toLong you get the error message

java.lang.AssertionError: expected: java.lang.Integer<20800301> but was: java.lang.Long<20800301> and there are 11 such errors. I don't claim that justify to introduce an extra function for it; just noting that Kotlin does not entirely solve it

[SanjaySargam]

Can I work on this issue?

[mikehardy]

Sorry for the delay! yes - appears to be open

[SanjaySargam]

Sorry for the delay! yes - appears to be open

Ok No problem

[SanjaySargam]

@mikehardy I am done with adding two functions in AnkiAssert.kt . When I tried to import it shows Add dependencies in AndkiDroid.unitTest. Where to add these dependencies

[mikehardy]

I don't know, I'm not familiar with this issue, or that error message, which appears incomplete to me? https://stackoverflow.com/help/how-to-ask

[BrayanDSO]

From https://github.com/ankidroid/Anki-Android/pull/12890#issuecomment-1329046742

I suggest discussing on the original issue more. Particularly, I agree with David that adding a new method for this isn't necessary in Kotlin. Plus, I don't see any benefit removing .toLong() from tests right now.

[github-actions[bot]]

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically