Kotlin-Polytech / KotlinAsFirst

Задачи для онлайн-курса "Котлин как первый язык программирования"
Creative Commons Attribution Share Alike 4.0 International
74 stars 115 forks source link

Different behaviour of kotoed and KoltinAsFirst with exception checking #85

Closed AbdullinAM closed 4 years ago

AbdullinAM commented 5 years ago

JUnit fuction assertThrows check the type of catched exception using isInstance method. In some cases it results in different behaviour of kotoed and local tests on the same input. Example from student solution of task lesson6.plusMinus:

fun plusMinus(expression: String): Int {
    val parts = expression.split(" ")
    val result = mutableListOf(parts[0].toInt())
    ...
    return result.sum()
}

If you run this solution on input string "~JD*_c\\teH:QnPQudT,8z" it will throw NumberFormatException. The requirement for this task is to throw IllegalArgumentException when input string has incorrect format.

If you will run local test using assertThrows, test will successfully pass, because NumberFormatException inherits IllegalArgumentException, therefore IllegalArgumentException::class.java.isInstance(NumberFormatException()) returns true.

If you will run the same solution in kotoed and random tests will generate similar input, the test will fail, because kotoed checks the exceptions like this:

modelException.javaClass == studentException.javaClass

This is really confusing to students, because they don't understand why the same solution works locally and fails on kotoed on the similar tests.

I think we need to add some custom assertThrows method to local tests that will behave like the checker in kotoed.

ice-phoenix commented 4 years ago

Fixed in kfirst-tests

belyaev-mikhail commented 4 years ago

Does this really fix this? In general, model solution could be throwing more specific exceptions, too

ice-phoenix commented 4 years ago

From what I've seen in random tests, in model impls we always throw the exact exception required in the task description.

We could add a parameter specifying the expected exception, but IMHO it would be an unnecessary complication at the moment.