search

uqbar-project / wollok

Wollok Programming Language
GNU General Public License v3.0
60 stars 16 forks source link

Que fallen los tests sin assert #1939

Closed PalumboN closed 3 years ago

PalumboN commented 4 years ago

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.

image

EDIT:

mmatos commented 4 years ago

Acabo de crear un issue que podría ser relevante a la hora de considerar este que me parece más interesante: https://github.com/uqbar-project/wollok/issues/1943

PalumboN commented 4 years ago

@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 commented 4 years ago

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)

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

matifreyre commented 4 years ago

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 notifications@github.com 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 https://github.com/uqbar-project/wollok/issues/1939#issuecomment-700343310, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPYIZSVARMWSYV6QLS6PIDSIEPCVANCNFSM4RBDPUEA .

mmatos commented 4 years ago

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 commented 4 years ago

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 notifications@github.com 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 https://github.com/uqbar-project/wollok/issues/1939#issuecomment-700702691, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPYIZTSAKPSKVQAP5EEOYTSIHOGTANCNFSM4RBDPUEA .

asanzo commented 4 years ago

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 notifications@github.com 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 < https://github.com/uqbar-project/wollok/issues/1939#issuecomment-700702691 , 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 https://github.com/uqbar-project/wollok/issues/1939#issuecomment-700708371, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJLXKBT32KUYLRGHDWNL2DSIHPGXANCNFSM4RBDPUEA .

asanzo commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

+1000 al doesNotThrowError

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

npasserini commented 3 years ago

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

asanzo commented 3 years ago

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 commented 3 years ago

B

fdodino commented 3 years ago

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

asanzo commented 3 years ago

Charlamos en la meet de hoy que queda la opción B, y se implementa en este issue: https://github.com/uqbar-project/wollok-language/issues/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)