Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unintuitive error description #120

Open
igorwojda opened this issue Nov 9, 2018 · 10 comments
Open

Unintuitive error description #120

igorwojda opened this issue Nov 9, 2018 · 10 comments

Comments

@igorwojda
Copy link

igorwojda commented Nov 9, 2018

class StringMaxCharTest {
    @Test
    fun `SOme test`() {
        maxChar("a") shouldEqual 'a'
    }
}

fun maxChar(str: String): Char? {
    val map = mutableMapOf<Char, Int>()

    str.forEach {
        map[it] = (map[it] ?: 0)  + 1
    }

    return map.maxBy { it.value }?.key
}

Above tests works fine, however when we change our assertion to compare with String, not Char:
maxChar("a") shouldEqual "a"

...tests fails with very unintuitive error description
java.lang.AssertionError: Expected <a>, actual <a>.

It's quite easy to make such a mistake by a programmer, so maybe Kluent can address this issue in a better way eg. by checking if the string has a single character and converting it to Char or display data types in error description

@MarkusAmshove
Copy link
Owner

This should be a problem within kotlin.test, as we're using their implementation of assertEquals in this case.

We could either raise an issue/pr on their side or write our own implementation and formatting as for #121

@igorwojda
Copy link
Author

I think aising a kotlin.test the issue would be a preferable thing to do. I am not that familiar with kotlin.test itself, so I will leave it up to you.

@danielgomezrico
Copy link

Another issue with this is that when you run the tests from the terminal it just prints AssertionError withouth the description...

DateTests > test date parse fromString changingTimeZones FAILED
    java.lang.AssertionError at DateTests.kt:20

@javatarz
Copy link
Contributor

javatarz commented Oct 5, 2019

@igorwojda: Can you check again? I think kotlin's assertEquals has been fixed.

Test code

@Test
fun beReadableWhenFailing() {
    val exception = assertFailsWith<AssertionError> { maxChar("a") shouldEqual "a" }
    val message = exception.message!!

    message shouldNotStartWith "expected: <a> but was: <a>"
    message shouldStartWith "expected: java.lang.String<a> but was: java.lang.Character<a>"
}

private fun maxChar(str: String): Char = str.map { it }.toSet().last()

Seems pretty clear

@igorwojda
Copy link
Author

@javatarz test still fails for me with the wrong exception. Here is my dependency:

testImplementation ("org.amshove.kluent:kluent-android:1.56") {
        exclude group: 'org.jetbrains.kotlin', module: 'kotlin-reflect'
        exclude group: 'org.jetbrains.kotlin', module: 'kotlin-stdlib-jdk8'
    }

@javatarz
Copy link
Contributor

javatarz commented Oct 8, 2019

@igorwojda I didn't try this on an independent project. I must admit, I added this test inside the Kluent repo (specifically, this was under the JVM module, not the android one).

Needs further investigation.

@igorwojda
Copy link
Author

You can easily reproduce this with my project.

  1. Checkout this https://github.com/igorwojda/kotlin-coding-puzzle
  2. Open in Android Studio
  3. Open MaxOccurrentChar.kt
  4. Replace file content with this
package com.igorwojda.string.maxchar

import org.junit.Test
import org.amshove.kluent.shouldEqual

class StringMaxCharTest {
    @Test
    fun `SOme test`() {
        maxChar("a") shouldEqual "a"
    }
}

fun maxChar(str: String): Char? {
    val map = mutableMapOf<Char, Int>()

    str.forEach {
        map[it] = (map[it] ?: 0)  + 1
    }

    return map.maxBy { it.value }?.key
}

  1. Run tests is StringMaxCharTest

@MarkusAmshove
Copy link
Owner

I'll have time to look at it at the weekend. @igorwojda you could try this on a new Android project and/or new JVM project and compare the outputs of gradle dependencies, there could be a discrepancy between Android and jvm kotlin versions

@drcolombo
Copy link
Contributor

If executing
"a" shouldBeEqualTo 'a'
and then getting

Unexpected type
Expected: <kotlin.Char> but was: <kotlin.String>
Expected :kotlin.Char
Actual :kotlin.String

is ok, then I will make a PR.
Basically, before comparing 'actual' and 'expected' we can check if types are the same. If not, then we will throw ComparisonFailedException with 'Unexpected type' message.

@MarkusAmshove
Copy link
Owner

I like that approach, a PR would be welcome :)

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

No branches or pull requests

5 participants