-
Notifications
You must be signed in to change notification settings - Fork 0
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
Objetivo 4 #33
Objetivo 4 #33
Conversation
Se añade en el fichero iv.yaml la ruta del archivo que contiene el código para ejecución de tests.
Se sigue la estrategia valor-error propia del lenguaje go. Se añaden métodos y estructuras para definir errores de cada clase.
Para empezar, muy buen curro. A simple vista lo veo bastante bien. Te ha faltado pegar en el cuerpo del PR el código que JJ pide en las nuevas entregas. Te lo dejo aquí. |
func (e *errorActividad) Error() string { | ||
return fmt.Sprintf("Error al crear la actividad: %s", e.err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tanto aquí como en zona.go podrías llevar los errores a un archivo a parte para usar mejores prácticas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Es cierto, pero por sencillez y mayor interacción con la terminal en la ejecución de los tests, opté por hacer un simple print en el shell. En un futuro seguramente lo cambie a un archivo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tampoco soy un experto en GO pero si ya atrapas los errores en las clases correspondientes de cada objeto, ¿por qué vuelves a poner un mensaje en el código con el que testeas?
No es ser experto en GO, es un concepto básico que había entendido mal sobre los tests del objetivo. Ahora los he corregido para que sean los correctos (aunque aparezcan menos) pero con más sentido.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
genial entonces!
docs/tests.md
Outdated
@@ -0,0 +1,14 @@ | |||
# Tests unitarios | |||
|
|||
Para la realización de los tests unitarios que se piden en el objetivo se ha creado un fichero denominado actividad_test.go en el que se declaran los tests correspondientes a la entidad Actividad y al objeto valor Zona. Se ha seguido el estándard de go que puede ser consultado [aquí](https://go.dev/doc/tutorial/add-a-test) en el que se hace uso de la biblioteca testing y se define la sintaxis que deben seguir las funciones: `TestXxxXXX (t *testing.T)`, entre otras cosas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En la documentación has dicho que has elegido la biblioteca testing, pero no has justificado la elección de esta. ¿Por qué has usado testing y no otra? ¿Qué requisitos tiene tu proyecto para haberla elegido?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listo, documentado aquí
|
||
Para la realización de los tests unitarios que se piden en el objetivo se ha creado un fichero denominado actividad_test.go en el que se declaran los tests correspondientes a la entidad Actividad y al objeto valor Zona. Se ha seguido el estándard de go que puede ser consultado [aquí](https://go.dev/doc/tutorial/add-a-test) en el que se hace uso de la biblioteca testing y se define la sintaxis que deben seguir las funciones: `TestXxxXXX (t *testing.T)`, entre otras cosas. | ||
|
||
Por otro lado se ha seguido el convenio de valor-error propio de go en las clases Actividad y Zona. Las funciones de creación de un objeto realizarán diversas comprobaciones, en el caso de haber un error, se devolverá el objeto y el error que ha ocurrido. En caso de que la ejecución no tenga ningún problema, se devolverá `nil`, que será el valor que se tomará de referencia para las comprobaciones en los tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¿Devolver nil está bien? ¿No se podría comprobar si se alza un error en lugar del valor devuelto?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realmente se devuelven dos valores, pero se comprueba el segundo en el caso de errores (se deshecha el primero haciendo uso de la variable _).
Respecto a nil, es el convenio de go para tratar los errores. Somos nosotros los que debemos asegurarnos de que si pasa "lo que no quieres que pase" se devuelva ese valor, que no es más que un valor indeterminado para punteros, funciones interfaces, mapas y slices.
Entiendo que te refieres a capturar los errores como se haría por ejemplo en Java, con un bloque try/catch, pero lametablemente eso no existe en Golang. Este lenguaje se caracteriza por tener errores estrictos que terminan el programa o funciones que permiten evitar el tratamiento de errores.
Te pongo un ejemplo:
Los desarrolladores de golang se suelen quejar de que las personas suelen hacer sentencias try/catch realmente extensas, que no siguen buenas prácticas de código limpio y sencillo. Por lo que esta función (que da error en tiempo de compilación si el fichero no existe)
f := os.Open("filename.txt")
Se trataría de la siguiente forma:
f, _ := os.Open("filename.txt")
Otro ejemplo que motiva este paradigma:
Imagina que en Java abres un fichero y empiezas a escribir en él, y llegado a un punto debes cerrar el fichero (debes cerrar el stream), por lo que deberías buscar en un posible try/catch extenso las opciones que deberías limpiar antes de cerrar.
En golang por ejemplo se usa la función defer(), que abre un fichero y simplemente lo cierra cuando ejecutas una función que deseas, por lo que te permite olvidar de los errores comunes por no cerrar un fichero, de que el fichero no exista, de que se te haya olvidado limpiar alguna opción, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No lo sabía, me parece supercurioso. Lo has explicado muy bien, me ha quedado clarísimo. Muchísimas gracias!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muchas gracias a ti también!!
PD: Tu avatar mola mucho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En general está muy bien, salvo la documentación que habría que detallarla un poco más y te he hecho también algunas sugerencias en los tests. Si no estás de acuerdo con algo dímelo, y si he dicho algo incorrecto debido a que no controlo el lenguaje dímelo sin miedo.
docs/tests.md
Outdated
@@ -0,0 +1,14 @@ | |||
# Tests unitarios | |||
|
|||
Para la realización de los tests unitarios que se piden en el objetivo se ha creado un fichero denominado actividad_test.go en el que se declaran los tests correspondientes a la entidad Actividad y al objeto valor Zona. Se ha seguido el estándard de go que puede ser consultado [aquí](https://go.dev/doc/tutorial/add-a-test) en el que se hace uso de la biblioteca testing y se define la sintaxis que deben seguir las funciones: `TestXxxXXX (t *testing.T)`, entre otras cosas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deberías justificar por qué utilizas la biblioteca testing. Está muy bien utilizada siguiendo estándares y tal pero ¿por qué esa biblioteca y no otra?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listo, documentado en el fichero corresopndiente.
"testing" | ||
) | ||
|
||
// ---------------------- Tests del Objeto Valor Zona ---------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me gusta la forma en la que gestionas los posibles errores dentro de las propias clases y también dentro de los tests, pero quizá deberías añadir test que comprueben el correcto funcionamiento, no sólo la buena gestión de errores. Por ejemplo, yo hice test que creaban objetos y se aseguraban (con assert) de que en efecto los campos importantes no estaban vacíos y el objeto estaba bien creado. Aún así me gustan tus test gestionando los errores, solo es una sugerencia para añadir otros tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entiendo tu punto de vista, y ciertamente lo comparto, pero es cierto que siguiendo el principio F.I.R.S.T. y haciendo los tests lo más sencillos y rápidos posibles, el uso de la función assert añadiría carga extra en algo que podemos considerar obvio, pues lo hace el compilador. Si introducimos un número como título de una actividad (que debe ser string), el propio compilador nos dará error para ello.
Es cierto que podría diseñar un test con la función assert que compruebe que efectivamente la zona que se ha proporcionado para la creación del objeto actividad es correcta. Esta función no está contemplada en el entorno normal de go, para su uso se debería añadir una biblioteca como testify, o crear una función assert propia.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sigo pensando que deberías comprobar el buen funcionamiento de alguna forma, pero entiendo tu punto de vista de querer hacer rápidos los tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hazle caso a @JAntonioVR . Realmente sólo estás testeando que se crean correctamente, no hay ninguna funcionalidad que estés testeando.
if titulo == "" { | ||
return act, &errorActividad{"titulo vacío"} | ||
} | ||
|
||
if categoria < Ocio || categoria > Deporte { | ||
return act, &errorActividad{"categoría no válida"} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muy buena gestión de errores, pero yo tendría en cuenta la posibilidad de que la zona que llega en el constructor sea nil
. Aunque por otra parte si termina de construirse es porque no peta el constructor de Zona (yo es que soy muy de dobles comprobaciones, pero es sólo una sugerencia).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Es cierto que se podría hacer, pero sería añadir código innecesario (además de que no es posible hacer lo que pides en este lenguaje de programación).
Surgen los siguientes problemas:
- nil es un valor indeterminado que se usa solo para punteros, interfaces, mapas, slices, canales y funciones. En este caso se usa en la interfaz
error
, pero no podría hacer lo mismo para el objeto de tipo Zona (salvo que lo enviase a la función como un puntero, cosa que no debería por el siguiente motivo). - El mero hecho de que el puntero sea indeterminado no aporta nada a la creación del objeto, puesto que si ha pasado por el constructor de Zona y ha sido validado (ha pasado los tests) es que se debe haber creado correctamente. Es lo que comentabas anteriormente de la doble comprobación.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cuestiones del lenguaje entonces, si es inviable o dificulta el desarrollo de los test solo queda confiar en el objeto de tipo Zona y en que no vaya a tomar valores incorrectos (y para eso están sus propios tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En general, está bastante correcto, aunque te falla la justificación técnica de la bilioteca de test elegida, revisa los comentarios que te he dejado por aquí abajo.
docs/tests.md
Outdated
@@ -0,0 +1,14 @@ | |||
# Tests unitarios | |||
|
|||
Para la realización de los tests unitarios que se piden en el objetivo se ha creado un fichero denominado actividad_test.go en el que se declaran los tests correspondientes a la entidad Actividad y al objeto valor Zona. Se ha seguido el estándard de go que puede ser consultado [aquí](https://go.dev/doc/tutorial/add-a-test) en el que se hace uso de la biblioteca testing y se define la sintaxis que deben seguir las funciones: `TestXxxXXX (t *testing.T)`, entre otras cosas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Como dicen mis compañeros, deberías de justificar por qué utilizas la biblioteca testing en algún lugar de la documentación. En cualquier justificación técnica siempre es necesario que establezcas unos requisitos antes de nada, realices una búsqueda parametrizada de los mismos y finalmente elijas aquel que mejor los cumpla.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listo, justificado en el archivo correspondiente.
|
||
Para la realización de los tests unitarios que se piden en el objetivo se ha creado un fichero denominado actividad_test.go en el que se declaran los tests correspondientes a la entidad Actividad y al objeto valor Zona. Se ha seguido el estándard de go que puede ser consultado [aquí](https://go.dev/doc/tutorial/add-a-test) en el que se hace uso de la biblioteca testing y se define la sintaxis que deben seguir las funciones: `TestXxxXXX (t *testing.T)`, entre otras cosas. | ||
|
||
Por otro lado se ha seguido el convenio de valor-error propio de go en las clases Actividad y Zona. Las funciones de creación de un objeto realizarán diversas comprobaciones, en el caso de haber un error, se devolverá el objeto y el error que ha ocurrido. En caso de que la ejecución no tenga ningún problema, se devolverá `nil`, que será el valor que se tomará de referencia para las comprobaciones en los tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisando la documentación de Go, veo que has seguido las mejores prácticas para la realización de tests, por esta parte 💯 .
Se añade información del principio F.I.R.S.T. y de la biblioteca usada para testeo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya casi por mi parte, una pequeña cuestión de estructura en la documentación y lo tienes
¿A qué te refieres con la estructura de la documentación? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Te dejo aquí la review que antes se me pasó
docs/tests.md
Outdated
El estándar de Go define por defecto el uso de la biblioteca testing, pero existen más alternativas: | ||
- **Testify**: Permite la definición de características para uso local y generación automática de datos para los tests. | ||
- **gocheck**: Define funciones de aserción, para, por ejemplo, la correcta definición de un objeto. | ||
- **Ginkgo and Gomega**: Se trata de un framework bastante pesado (pero con muchas utilidades) de testeo de BDD y funciones de aserción. | ||
- **GoConvey**: Otro framework de testeo de BDD con una interfaz web de fácil uso. | ||
|
||
La elección de testing viene motivada por dos motivos principales: | ||
- **Documentación oficial**: La documentación oficial de go hace uso de ella por su simplez y porque viene ya integrada en el propio entorno de go. Esto promueve que sea fácil de utilizar (ya que es fácilmente localizable en la documentación de go) y que no sea necearia instalaciones adicionales. | ||
- **Sencillez del proyecto**: En este caso la sencillez de la aplicación permite el uso de cualquiera de las bibliotecas habilitadas para testeo, pero tiene menor sentido hacer uso de frameworks pesados llenos de utilizadades que no usaremos. | ||
|
||
Realmente la diferencia radicaba entre usar Testify y testing para este proyecto, y nos decantamos por testing por seguir los estándares marcados para go (que no quiere decir que sea lo óptimo) y por la sencillez de los tests, además de no tener la necesidad de hacer uso de funciones especiales. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muy buena justificación, pero deberías hacer la comparación de alternativas antes de comunicar la decisión final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listo, es verdad, es mejor cambiar la estructura para que quede antes la comparativa que la decisión final
Te lo acabo de poner, fallo mío antes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Palantísimo 👍 🎉
Yo lo veo bastante bien ya 😄 |
En primer lugar gracias a todos, ha sido una gran ayuda y he cambiado bastantes cosas que no estaban bien (documentación y tests sobre todo). @lentes4k @alexespana Si lo veis bien vosotros también aprobad los cambios y avisaré al profesor esta tarde para la corrección. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buen curro y muy buenas explicaciones aquí en el PR 😄
@JJ Estaría listo para revisión. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La verdad es que habéis hecho un currazo encomiable, pero falta algo fundamental: hay que tener completa, y testear, la lógica de negocio o parte de ella.
Se añade información relacionada a la lógica de negocio. Avanza la lógica de negocio gracias a los tests desarrollados.
Es totalmente cierto, cuando he leído el mensaje se me ha venido a la cabeza. Hace tiempo cerré la primera Historia de Usuario creyendo haberla finalizado, pero no ha sido hasta ahora que nos podemos asegurar que dicha historia puede darse por completada. Gracias a los tests nos aseguramos la correcta definición y creación de una actividad, que es la entidad principal con la que se relaciona la lógica de negocio. Sin hacer referencia a dicha HU, los cambios quedaban un poco "sueltos" porque no se especificaba el por qué se están validando esos datos. Todo ello ha sido explicado en el fichero tests.md y README.md, además de estar más desarrollado. También se ha hecho mención en el cuerpo del PR. Por otro lado se ha movido la Historia de Usuario correspondiente al hito en el que se está trabajando, que es a donde se corresponde. Como ya ha explicado en otras ocasiones, las Historias de Usuario se mueven a lo largo de los hitos. Respecto a la experiencia de trabajo grupal, ha sido bastante gratificante y muy entretenido, pero sobre todo es por el rápido feedback que se ha tenido por parte de mis compañeros. @JJ Ahora estaría listo para revisión. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realmente es el mismo comentario que antes: debes crear el código de la lógica de negocio, o parte, y testearla. También asegúrate que este código está en un PMV y que este describe correctamente lo que va a resultar de esto.
@@ -7,7 +7,7 @@ La idea principal es desarrollar una aplicación en la que las personas puedan o | |||
|
|||
Por otro lado, se pretende establecer una clasificación en diferentes categorías (deportes, actividades culturales, etc.) para facilitar la búsqueda y creación a los usuarios. | |||
|
|||
Esta aplicación dispondría de un sistema en el que cualquier usuario podría crear una actividad con ciertas características y otros usuarios pudiesen apuntarse a ella. Por otro lado, para facilitar la comunicación, la aplicación crearía un grupo de chat al que se unirían automáticamente los nuevos miembros de la actividad. | |||
Esta aplicación dispondría de un sistema en el que cualquier usuario podría crear una actividad con ciertas características y otros usuarios pudiesen apuntarse a ella, consiguiendo por tanto, que diferentes usuarios se conociesen entre sí y pudiesen realizar la actividad que de otra forma no hubiese sido posible, que a su vez, representa la lógica de negocio de la aplicación. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me temo que es el código de la lógica de negocio lo que tienes que desarrollar. Tampoco un chat es realmente lógica de negocio. Repasa el concepto, por favor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El chat era un vestigio de un anterior objetivo que se me pasó quitar, lo comentamos en clase un día y decidí quitarlo.
"testing" | ||
) | ||
|
||
// ---------------------- Tests del Objeto Valor Zona ---------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hazle caso a @JAntonioVR . Realmente sólo estás testeando que se crean correctamente, no hay ninguna funcionalidad que estés testeando.
Se ha añadido un método que compara dos zonas, atributo por atributo.
Se han modificado los tests para que se compruebe funcionalidad. Se ha añadido test para la búsqueda por zona.
Creo que lo estaba entendiendo mal, simplemente estaba testeando (en la creación de Zona y Actividad) que el objeto no admitía que un argumento tuviese "x" estructura (por ejemplo, que no fuese una cadena vacía), pero en ningún momento testeaba si ese objeto realmente se había creado con la información que yo quería. Se ha resuelto este problema. Por otro lado, me ha parecido pobre simplemente la comprobación de dos constructores que por sí solos no pueden desarrollar la lógica de negocio (esto es lo que estaba entendiendo mal). Para resolver esto, he añadido también varias funcionalidades y nuevas issues #37 #38 junto con un nuevo test. Por último, se ha añadido información referente al último test en el archivo tests.md. @JJ Perdón por la molestia, espero que ya esté listo para pasar el objetivo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 bien currado por parte de todo el mundo.
No tienes por qué poner en el milestone que "incluya tests". Va implícito que hay que testearlo con los criterios de viabilidad que incluyas.
Para la realización del objetivo 4 se han hecho los siguientes cambios:
Lista de comprobación
usuario que desarrolla, pasando por el mensaje de commit y el issue
correspondiente? Sí, todos los commits están referidos a issues, que a su vez, están enlazados con historias de usuario.
esté desarrollando? Sí, el principal objetivo es la validación del objeto Actividad, que está ligado con la lógica de negocio y que está reflejado por la Historia de Usuario [HU1] - Usuario - Registro de una actividad #13 .
este objetivo? Sí, el PMV está descrito en el hito correspondiente.
módulos o paquetes más fundamentales frente a otros, movidos a otro
PMV/Hito? Puesto que solo había una entidad y un objeto valor (del que dependía la identidad), los tests se han realizado sobre ambos.
de test runner? Sí, se ha documentado aquí.
búsqueda para las diferentes opciones? Sí, se puede ver en docs/test.md
se ha documentado cómo se ha hecho? Sí, se ha documentado aquí.
considere viable? Sí, se encuentra en la descripción del hito correspondiente.
Modificaciones realizadas
Con los anteriores cambios se satisfacen las HUs #27 , #13 y #14 y se completa el hito 3, finalizando a su vez el registro de una actividad, fundamental para satisfacer la lógica de negocio de la aplicación.