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

Checking expected timeouts on check_error too - Fixes mumuki/mumuki-laboratory#1207 #127

Merged
merged 2 commits into from
Oct 4, 2018

Conversation

afska
Copy link
Collaborator

@afska afska commented Oct 3, 2018

Fixes mumuki/mumuki-laboratory#1207

Este PR es sobre los errores:

  1. Otro problema más raro aún es que en https://mumuki.io/primaria/exercises/5589-repeticion-condicional-practica-al-infinito, si se envíe el código tal cual está, da verde (pero debería fallar, porque falta un bloque)

Esto sucedía por varios problemas:

  • gobstones-blockly generaba while(not(())) { ... } con el código en cuestión
  • () es una tupla vacía en XGobstones
  • not de una tupla vacía es un error en tiempo de ejecución, da boom
  • El ejercicio tenía un solo initial_board pero ningún test, entonces no rompía. Si hubiera sido:
expect_endless_while: true
check_head_position: false
examples:
 - initial_board: |
     GBB/1.0
     size 2 2
     cell 0 1 Negro 4 Rojo 4 
     cell 1 1 Azul 6 Rojo 4 
     cell 0 0 Negro 4 Rojo 4 
     cell 1 0 Negro 4 Rojo 4 
     head 1 1
   final_board: |
     GBB/1.0
     size 2 2
     head 0 0

... habría funcionado.

Con este PR ahora también se puede hacer:

expect_endless_while: true
check_head_position: false
examples:
 - initial_board: |
     GBB/1.0
     size 2 2
     cell 0 1 Negro 4 Rojo 4 
     cell 1 1 Azul 6 Rojo 4 
     cell 0 0 Negro 4 Rojo 4 
     cell 1 0 Negro 4 Rojo 4 
     head 1 1
   error: "timeout"

...lo cual es más cortito y también aviva al runner de que hay un test que correr.

⚠️ Mergear esto es un chiche nomás. Se puede hacerlo andar de la primera forma sin necesidad de hacer otro deploy.

  1. La UI se congela al correr un ejercicio con loops infinitos

Esto lo estuve viendo pero no es tan fácil, la forma de solucionarlo para mí es con Web Workers pero hay varios temas a solucionar:

  • No pueden acceder al scope global, ni a ningún dato que no se pase mediante .postMessage(...) que solo admite datos planos y no objetos con comportamiento.
  • Pueden importar scripts (en este caso el código del intérprete) pero solo mediante un importScripts que los baja de la red, y esto desde el runner se complica porque corren dentro de labo
  • Habría que hacer un refactor grande en gobstones-code-runner para hacer el parseo e interpretación en el mismo paso y que la interfaz sea siempre asincrónica

Igualmente lo veo posible, pero es una tarea de varios días así que lo cargué en un issue aparte (#128)

@afska afska changed the title Checking expected timeouts on check_error too Checking expected timeouts on check_error too - Fixes mumuki/mumuki-laboratory#1207 Oct 3, 2018
@ghost ghost assigned afska Oct 3, 2018
@flbulgarelli
Copy link
Member

Hola! Excelente y clarísimo. Mi única duda es: si ahora tenemos el errror-timeout como una opción más, ¿qué aporta el expect-endless-while? ¿No es redundante?

@afska
Copy link
Collaborator Author

afska commented Oct 3, 2018 via email

@flbulgarelli
Copy link
Member

Ok. Esta bien, no es necesario. Era solo para entender.

Me gustaría que el expect-endless-while siga a andando por retrocompatibilidad, pero que no sea necesario si pones error timeout. O viceversa.

@ghost ghost removed the in progress label Oct 4, 2018
@afska
Copy link
Collaborator Author

afska commented Oct 4, 2018

@flbulgarelli con el último cambio el check_endless_while ya no sería necesario. Si alguno de los examples chequea por el error "timeout", no retorna más :aborted como antes.

@flbulgarelli flbulgarelli merged commit 5cba1e8 into master Oct 4, 2018
@ghost ghost removed the pending-review label Oct 4, 2018
@flbulgarelli
Copy link
Member

¡Excelente!

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

Successfully merging this pull request may close these issues.

2 participants