Que fallen los tests sin assert

[PalumboN]

Actualmente ya tenemos un warning cuando no se le manda ningún mensaje al objeto assert dentro de un test.

Me gustaría que además el test fallara (amarillo) diciendo que no se assertó nada.

---

EDIT:

• Depende de que podamos tener doesNotThrowException Agregar a los tests el assert "esto no debe fallar" (doesNotThrowException) wollok-language#82
• Depende de que podamos detectar los assert en los methods del describe Validación del uso de assertions no detecta si el assert está dentro de un método del describe #1943

[mmatos]

Acabo de crear un issue que podría ser relevante a la hora de considerar este que me parece más interesante: #1943

[PalumboN]

@asanzo el otro día mostró en clase un uso de testing donde en este caso se espera que el test pase (comportamiento actual, el de la imagen) que me hizo dudar de mi propuesta.

[asanzo]

Estoy de acuerdo con @mmatos de que ese chequeo debería considerar los métodos de los tests, pero como dice @PalumboN me parece que lo que necesitamos es unassert.everythingWentFine() o similar. Paso a explicar un ejemplo similar al que vimos en clase:

Me explico: vos esperás hacer 4 tests con un método cuenta1.transferir(1000,cuenta2)

• "Al transferir desde una cuenta sin guita explota"
• "Al transferir desde una cuenta la cuenta origen queda con menos guita"
• "al transferir hacia una cuenta la cuenta destino queda con más guita".
• "Al transferir desde una cuenta con guita no debería explotar" <- este test es distinto de los 3 anteriores, y si bien está cubierto en los dos anteriores, es didácticamente interesante poderlo hacer. Y estaría bueno tener un assert que me deje ser declarativo al respecto. ¿Quizás un assert.doesNotThrowException({cuenta1.transferir(1000,cuenta2)}) es más declarativo?

Como dice el rasta, creo que para poder implementar el "que fallen los tests sin assert" necesitamos considerar casos como este.

[matifreyre]

Yo eso lo hago con un catch y un assert.fail("sarasa"). OK, podría haber algo que no requiera catch, aunque no sé qué tan necesario sea.

…

On Mon, Sep 28, 2020 at 8:56 PM asanzo ***@***.***> wrote: Estoy de acuerdo con @mmatos <https://github.com/mmatos> de que ese chequeo debería considerar los métodos de los tests, pero como dice @PalumboN <https://github.com/PalumboN> me parece que lo que necesitamos es unassert.everythingWentFine(). Paso a explicar un ejemplo similar al que vimos en clase: Me explico: vos esperás hacer 4 tests con un método cuenta1.transferir(1000,cuenta2) - "Al transferir desde una cuenta sin guita explota" - "Al transferir desde una cuenta la cuenta origen queda con menos guita" - "al transferir hacia una cuenta la cuenta destino queda con más guita". - *"Al transferir desde una cuenta con guita no debería explotar"* <- este test es distinto de los 3 anteriores, y si bien está cubierto en los dos anteriores, es didácticamente interesante poderlo hacer. Y estaría bueno tener un assert que me deje ser declarativo al respecto. ¿Quizás un assert.doesNotThrowException({cuenta1.transferir(1000,cuenta2)})? Como dice el rasta, creo que para poder implementar el "que fallen los tests sin assert" necesitamos considerar casos como este. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1939 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACPYIZSVARMWSYV6QLS6PIDSIEPCVANCNFSM4RBDPUEA> .

[mmatos]

Es necesario si mostrás self.error("..") y todavía no hablaste de try/catch.

+1 a tener un assertion como el que propone Alf (similar al expect { }.not_to raise_error de RSpec)

[matifreyre]

Ah, claro... es que nosotros damos todo junto :-D ... En ese caso, `assert.succeed()` o `assert.success()` serían mis candidatos.

…

On Tue, Sep 29, 2020 at 10:28 AM Mariana ***@***.***> wrote: Es necesario si mostrás self.error("..") y todavía no hablaste de try/catch. +1 a tener un assertion como el que propone Alf (similar al expect { }.not_to raise_error de RSpec) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1939 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACPYIZTSAKPSKVQAP5EEOYTSIHOGTANCNFSM4RBDPUEA> .

[asanzo]

Yo no doy try catch más que como bonus así que no es una solución viable :P El mar., 29 sept. 2020 a las 10:36, Matías Freyre (<notifications@github.com>) escribió:

…

Ah, claro... es que nosotros damos todo junto :-D ... En ese caso, `assert.succeed()` o `assert.success()` serían mis candidatos. On Tue, Sep 29, 2020 at 10:28 AM Mariana ***@***.***> wrote: > Es necesario si mostrás self.error("..") y todavía no hablaste de > try/catch. > > +1 a tener un assertion como el que propone Alf (similar al expect { > }.not_to raise_error de RSpec) > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > < #1939 (comment) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/ACPYIZTSAKPSKVQAP5EEOYTSIHOGTANCNFSM4RBDPUEA > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1939 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJLXKBT32KUYLRGHDWNL2DSIHPGXANCNFSM4RBDPUEA> .

[asanzo]

Volviendo a ver esto, me gusta un poco menos la propuesta de @matifreyre .

Me gusta más esto:

test "Al transferir desde una cuenta con guita no debería explotar" {
     assert.doesNotThrowError({cuentaConGuita.transferirA(cuentaSinGuita, 1000)})
}

que esto:

test "Al transferir desde una cuenta con guita no debería explotar" {
     cuentaConGuita.transferirA(cuentaSinGuita, 1000)
     assert.success()
}

El primero me parece más declarativo, y tiene una sintaxis parecida a la familia assert.throwsError, mientras que el segundo requiere hacer el paso de entender que success en realidad está ahí porque si llega ahí es porque no explotó....

[fdodino]

A mí me resulta simpático doesNotThrowError, se puede implementar en Wollok mismo, en cuyo caso lo podemos mover a wollok-language y sería un PR fantástico para hacer, guiño guiño...

@asanzo @mmatos @matifreyre @PalumboN , si me dan el ok lo paso a wollok-language...

[PalumboN]

+1000 al doesNotThrowError

Pero lo llamaría notThrowsException porque consistencia con los métodos que ya tenemos > consistencia con el inglés 👀

[npasserini]

Por favor no pongamos cosas mal escritas en inglés. doesNotThrowExceptiondebería ser good enough.

[asanzo]

1. ¿Qué les va más?

// Opción A
cuenta1.transferir(1000,cuenta2)
assert.doesNotThrowException()

// Opción B
assert.doesNotThrowException({cuenta1.transferir(1000,cuenta2)})

2. Segunda pregunta: ¿Entonces agregamos también la propuesta de rasta que el test de amarillo?

[tesonep]

B

[fdodino]

Opción B y sí. +1 a crear el doesNotThrowException en un issue de wollok-language y apuntarlo desde acá

[asanzo]

Charlamos en la meet de hoy que queda la opción B, y se implementa en este issue: uqbar-project/wollok-language#82

Este issue queda para lo que originalmente propuso @PalumboN (que el test runner dé amarillo al correr un test sin asserts), ahí edité el comment ppal agregando las dependencias que habría que resolver antes que este.

@fdodino decime si quedaron los issues suficientemente claros como querías, y también porfa péguenle una revisada a las dependencias quienes estuvimos discutiendo acá (especialmente @mmatos que no estuvo en la meet de hoy)