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

Objetivo 4 #33

Merged
merged 19 commits into from
Dec 17, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,7 @@ Una vez que hemos clonado el repositorio y estamos dentro de él, podemos utiliz
* **task check** --> Comprueba la sintaxis de las entidades programadas.
* **task installdeps** --> Incluye las dependencias que se encuentran en go.mod a la hora de instalar o ejecutar el programa.
* **task test** --> Ejecuta todos los tests del proyecto.

## Test unitarios

Toda la información referente a los tests unitarios desarrollados para este proyecto se puede encontrar [aquí](docs/tests.md).
14 changes: 14 additions & 0 deletions docs/tests.md
Original file line number Diff line number Diff line change
@@ -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.

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?

Copy link
Owner Author

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.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listo, documentado aquí

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.

Copy link
Owner Author

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.


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.

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?

Copy link
Owner Author

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.

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!

Copy link
Owner Author

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

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 💯 .


## Uso de Task

Gracias al task runner elegido en el objetivo 3, podemos automatizar la ejecución de tests con la siguiente orden:
`task test`

Nos aparecerá por pantalla la ejecución de cada test y su duración.

Por último añadir que nuestro archivo actividad_test.go se encuentra en `/pkg/actividad/actividad_test.go` y en el archivo Taskfile.yml se encuentra la ruta `/pkg/...`. Es por ello que el anterior comando funciona correctamente.
1 change: 1 addition & 0 deletions iv.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ entidad: pkg/actividad/actividad.go
automatizar:
fichero: Taskfile.yml
orden: task
test: pkg/actividad/actividad_test.go
26 changes: 24 additions & 2 deletions pkg/actividad/actividad.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// representar una actividad organizada por un usuario.
package actividad

import "fmt"

// Representa las distintas categorías de actividades
type Categoria int

Expand All @@ -24,7 +26,27 @@ type Actividad struct {
categoria Categoria
}

type errorActividad struct {
err string
}

func (e *errorActividad) Error() string {
return fmt.Sprintf("Error al crear la actividad: %s", e.err)
}

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

Copy link
Owner Author

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.

Copy link
Owner Author

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

genial entonces!

// NewActividad inicializa y devuelve un objeto Actividad
func NewActividad(titulo string, zona Zona, categoria Categoria) Actividad {
return Actividad{titulo, zona, categoria}
func NewActividad(titulo string, zona Zona, categoria Categoria) (Actividad, error) {
var act Actividad

if titulo == "" {
return act, &errorActividad{"titulo vacío"}
}

if categoria < Ocio || categoria > Deporte {
return act, &errorActividad{"categoría no válida"}
}
Comment on lines +41 to +47

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).

Copy link
Owner Author

@XileonXL XileonXL Dec 16, 2021

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.

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)


act = Actividad{titulo, zona, categoria}

return act, nil
}
44 changes: 44 additions & 0 deletions pkg/actividad/actividad_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package actividad

import (
"testing"
)

// ---------------------- Tests del Objeto Valor Zona ----------------------

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.

Copy link
Owner Author

@XileonXL XileonXL Dec 16, 2021

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.

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.

Copy link

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.

func TestLocalidadNoVacia(t *testing.T) {
_, err := NewZona("", "Granada", "España")
if err == nil {
t.Fatalf("Localidad no válida en objeto Zona")
}
}

func TestProvinciaNoVacia(t *testing.T) {
_, err := NewZona("Motril", "", "España")
if err == nil {
t.Fatalf("Provincia no válida en objeto Zona")
}
}

func TestPaisNoVacio(t *testing.T) {
_, err := NewZona("Motril", "Granada", "")
if err == nil {
t.Fatalf("País no válido en objeto Zona")
}
}

// ---------------------- Tests de la Entidad Actividad ----------------------
func TestTituloNoVacio(t *testing.T) {
zona := Zona{"Motril", "Granada", "España"}
_, err := NewActividad("", zona, Ocio)
if err == nil {
t.Fatalf("Título no válido en objeto Actividad")
}
}

func TestCategoriaNoValida(t *testing.T) {
zona := Zona{"Motril", "Granada", "España"}
_, err := NewActividad("Ruta de senderismo", zona, 99)
if err == nil {
t.Fatalf("Categoría no válida en objeto Actividad")
}
}
30 changes: 28 additions & 2 deletions pkg/actividad/zona.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package actividad

import "fmt"

// Zona representa una localidad en la que puede organizarse
// una actividad
type Zona struct {
Expand All @@ -13,7 +15,31 @@ type Zona struct {
pais string
}

type errorZona struct {
err string
}

func (e *errorZona) Error() string {
return fmt.Sprintf("Error al crear la zona: %s", e.err)
}

// NewZona inicializa y devuelve un objeto Zona
func NewZona(localidad string, provincia string, pais string) Zona {
return Zona{localidad, provincia, pais}
func NewZona(localidad string, provincia string, pais string) (Zona, error) {
var zona Zona

if localidad == "" {
return zona, &errorZona{"localidad vacía"}
}

if provincia == "" {
return zona, &errorZona{"provincia vacía"}
}

if pais == "" {
return zona, &errorZona{"país vacío"}
}

zona = Zona{localidad, provincia, pais}

return zona, nil
}