test: pruebas unitarias para middleware ValidarSistema#37
Open
IgorMaltsov wants to merge 4 commits intogaligaribaldi:mainfrom
Open
test: pruebas unitarias para middleware ValidarSistema#37IgorMaltsov wants to merge 4 commits intogaligaribaldi:mainfrom
IgorMaltsov wants to merge 4 commits intogaligaribaldi:mainfrom
Conversation
Reviewer's GuideAdds focused unit tests for the ValidarSistema middleware and promotes the CORS dependency to a direct requirement, ensuring correct behavior for valid/invalid systems and verified context values. Sequence diagram for ValidarSistema middleware request flowsequenceDiagram
actor Client
participant GinRouter
participant MiddlewareValidarSistema
participant Handler
Client->>GinRouter: HTTP GET /:sistema/...
GinRouter->>MiddlewareValidarSistema: Invoke with context
MiddlewareValidarSistema->>MiddlewareValidarSistema: Read path param sistema
alt sistema is valid (METRO, MB, CBB, MEXIBUS, others)
MiddlewareValidarSistema->>MiddlewareValidarSistema: Normalize to uppercase
MiddlewareValidarSistema->>GinRouter: Set context key sistemaValidado
GinRouter->>Handler: Call next handler with context
Handler-->>Client: 200 OK
else sistema is invalid (unknown, empty, numeric)
MiddlewareValidarSistema-->>Client: 404 Not Found
end
Class diagram for ValidarSistema middleware and testsclassDiagram
class MiddlewarePackage {
+ValidarSistema() gin.HandlerFunc
}
class GinContext {
+Param(key string) string
+Next()
+AbortWithStatus(code int)
+Set(key string, value interface_any)
}
class MiddlewareTestSuite {
+TestValidSystems()
+TestInvalidSystems()
+TestMexibusWithoutAccent()
+TestContextStoresUppercaseSistema()
}
MiddlewarePackage ..> GinContext : uses
MiddlewareTestSuite ..> MiddlewarePackage : tests
MiddlewareTestSuite ..> GinContext : mocks or uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Including the known bug case ("MEXIBUS es no valida", 404) in the same common test table makes it harder to distinguish regressions from intentional failures; consider separating this into its own test or clearly marking it as a bug-expectation so future behavior changes can be handled explicitly.
- In
TestValidarSistema_ContextValue, add an assertion on the HTTP status code before decoding the body so that JSON decoding errors or unexpected responses are caught more clearly rather than failing only on the decoded value. - Using
gin.SetMode(gin.TestMode)in aninitfunction changes global state for all tests; consider moving this to a test helper orTestMainso its scope and impact on other packages’ tests are explicit and easier to control.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Including the known bug case ("MEXIBUS es no valida", 404) in the same common test table makes it harder to distinguish regressions from intentional failures; consider separating this into its own test or clearly marking it as a bug-expectation so future behavior changes can be handled explicitly.
- In `TestValidarSistema_ContextValue`, add an assertion on the HTTP status code before decoding the body so that JSON decoding errors or unexpected responses are caught more clearly rather than failing only on the decoded value.
- Using `gin.SetMode(gin.TestMode)` in an `init` function changes global state for all tests; consider moving this to a test helper or `TestMain` so its scope and impact on other packages’ tests are explicit and easier to control.
## Individual Comments
### Comment 1
<location path="cmd/pkg/controller/middleware/middleware_test.go" line_range="34-37" />
<code_context>
+func TestValidarSistema_Common(t *testing.T) {
+ router := setupRouter()
+
+ cases := []struct {
+ descripcion string
+ sistem string
+ codigoEsperado int
+ }{{"Metro es valid", "METRO", 200},
+ {"MB es valida", "MB", 200},
+ {"metro es valid", "metro", 200},
</code_context>
<issue_to_address>
**suggestion (testing):** Falta un caso de prueba para el sistema `CBB` (y quizá otros) que se menciona en la descripción del PR
En la descripción del PR indicas que agregas casos válidos para METRO, MB, CBB y otros, pero en las pruebas sólo están METRO, MB y MEXIBÚS/MEXIBUS. Propongo añadir al menos un caso explícito para `CBB` (y cualquier otro sistema que se considere soportado) para que las pruebas coincidan con el alcance declarado y verifiquen esos caminos en el middleware.
```suggestion
}{{"Metro es valid", "METRO", 200},
{"MB es valida", "MB", 200},
{"CBB es valido", "CBB", 200},
{"metro es valid", "metro", 200},
{"MEXIBÚS es valida", "MEXIBÚS", 200}, //Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?.
```
</issue_to_address>
### Comment 2
<location path="cmd/pkg/controller/middleware/middleware_test.go" line_range="36-39" />
<code_context>
+ }{{"Metro es valid", "METRO", 200},
+ {"MB es valida", "MB", 200},
+ {"metro es valid", "metro", 200},
+ {"MEXIBÚS es valida", "MEXIBÚS", 200}, //Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?.
+ {"MEXIBUS es no valida", "MEXIBUS", 404},
+ {"Cadena vacia es no valida", "", 404},
+ {"Numero es no valida", "1", 404},
</code_context>
<issue_to_address>
**suggestion (testing):** Aclarar la intención del caso `MEXIBUS` para que el test no se vuelva engañoso cuando se arregle el bug
Este caso está codificando como comportamiento esperado un bug conocido (MEXIBUS sin acento devuelve 404). Cuando alguien lo corrija, el test fallará pero no quedará claro si el 404 era lo deseado o solo el estado actual. Sugiero marcar explícitamente que es un bug conocido (p. ej. en el nombre de la subprueba o con un comentario `TODO: cambiar a 200 cuando se soporte MEXIBUS sin acento`), o separarlo en una prueba distinta que indique que el comportamiento es indeseado.
```suggestion
{"metro es valid", "metro", 200},
{"MEXIBÚS es valida", "MEXIBÚS", 200}, // Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?.
// TODO: cambiar a 200 cuando se soporte "MEXIBUS" sin acento; actualmente documenta un bug conocido.
{"MEXIBUS sin acento devuelve 404 (bug conocido)", "MEXIBUS", 404},
{"Cadena vacia es no valida", "", 404},
```
</issue_to_address>
### Comment 3
<location path="cmd/pkg/controller/middleware/middleware_test.go" line_range="58-67" />
<code_context>
+ }
+}
+
+func TestValidarSistema_ContextValue(t *testing.T) {
+ router := setupRouter()
+
+ t.Run("ToUpper es ok", func(t *testing.T) {
+ w := httptest.NewRecorder()
+
+ req, _ := http.NewRequest("GET", "/"+"metro"+"/test", nil)
+
+ router.ServeHTTP(w, req)
+
+ var body map[string]string
+
+ err := json.NewDecoder(w.Body).Decode(&body)
+
+ if err != nil {
+ t.Errorf("Error al decodificar el cuerpo de la respuesta %v", err)
+ }
+
+ if body["sistema"] != "METRO" {
+ t.Errorf("TopUpper no trabajo %v", body["sistema"])
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** En la prueba de contexto también conviene verificar el código de estado y la presencia de la clave
Además de validar el valor en mayúsculas, sería útil (1) comprobar explícitamente que el código de respuesta sea 200 antes de decodificar el cuerpo, y (2) verificar que la clave "sistema" exista en el mapa usando el segundo valor de la indexación (`value, ok := body["sistema"]`). Así la prueba queda más robusta ante cambios en el handler o el middleware.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+34
to
+37
| }{{"Metro es valid", "METRO", 200}, | ||
| {"MB es valida", "MB", 200}, | ||
| {"metro es valid", "metro", 200}, | ||
| {"MEXIBÚS es valida", "MEXIBÚS", 200}, //Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?. |
There was a problem hiding this comment.
suggestion (testing): Falta un caso de prueba para el sistema CBB (y quizá otros) que se menciona en la descripción del PR
En la descripción del PR indicas que agregas casos válidos para METRO, MB, CBB y otros, pero en las pruebas sólo están METRO, MB y MEXIBÚS/MEXIBUS. Propongo añadir al menos un caso explícito para CBB (y cualquier otro sistema que se considere soportado) para que las pruebas coincidan con el alcance declarado y verifiquen esos caminos en el middleware.
Suggested change
| }{{"Metro es valid", "METRO", 200}, | |
| {"MB es valida", "MB", 200}, | |
| {"metro es valid", "metro", 200}, | |
| {"MEXIBÚS es valida", "MEXIBÚS", 200}, //Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?. | |
| }{{"Metro es valid", "METRO", 200}, | |
| {"MB es valida", "MB", 200}, | |
| {"CBB es valido", "CBB", 200}, | |
| {"metro es valid", "metro", 200}, | |
| {"MEXIBÚS es valida", "MEXIBÚS", 200}, //Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?. |
Comment on lines
+36
to
+39
| {"metro es valid", "metro", 200}, | ||
| {"MEXIBÚS es valida", "MEXIBÚS", 200}, //Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?. | ||
| {"MEXIBUS es no valida", "MEXIBUS", 404}, | ||
| {"Cadena vacia es no valida", "", 404}, |
There was a problem hiding this comment.
suggestion (testing): Aclarar la intención del caso MEXIBUS para que el test no se vuelva engañoso cuando se arregle el bug
Este caso está codificando como comportamiento esperado un bug conocido (MEXIBUS sin acento devuelve 404). Cuando alguien lo corrija, el test fallará pero no quedará claro si el 404 era lo deseado o solo el estado actual. Sugiero marcar explícitamente que es un bug conocido (p. ej. en el nombre de la subprueba o con un comentario TODO: cambiar a 200 cuando se soporte MEXIBUS sin acento), o separarlo en una prueba distinta que indique que el comportamiento es indeseado.
Suggested change
| {"metro es valid", "metro", 200}, | |
| {"MEXIBÚS es valida", "MEXIBÚS", 200}, //Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?. | |
| {"MEXIBUS es no valida", "MEXIBUS", 404}, | |
| {"Cadena vacia es no valida", "", 404}, | |
| {"metro es valid", "metro", 200}, | |
| {"MEXIBÚS es valida", "MEXIBÚS", 200}, // Creo que en producción, las solicitudes se enviarán utilizando la opción "MEXIBUS"?. | |
| // TODO: cambiar a 200 cuando se soporte "MEXIBUS" sin acento; actualmente documenta un bug conocido. | |
| {"MEXIBUS sin acento devuelve 404 (bug conocido)", "MEXIBUS", 404}, | |
| {"Cadena vacia es no valida", "", 404}, |
Comment on lines
+58
to
+67
| func TestValidarSistema_ContextValue(t *testing.T) { | ||
| router := setupRouter() | ||
|
|
||
| t.Run("ToUpper es ok", func(t *testing.T) { | ||
| w := httptest.NewRecorder() | ||
|
|
||
| req, _ := http.NewRequest("GET", "/"+"metro"+"/test", nil) | ||
|
|
||
| router.ServeHTTP(w, req) | ||
|
|
There was a problem hiding this comment.
suggestion (testing): En la prueba de contexto también conviene verificar el código de estado y la presencia de la clave
Además de validar el valor en mayúsculas, sería útil (1) comprobar explícitamente que el código de respuesta sea 200 antes de decodificar el cuerpo, y (2) verificar que la clave "sistema" exista en el mapa usando el segundo valor de la indexación (value, ok := body["sistema"]). Así la prueba queda más robusta ante cambios en el handler o el middleware.
Owner
|
Hola igor. |
galigaribaldi
added
help wanted
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
¡Hola! Estoy comenzando mi camino en la automatización de pruebas con Go.
Encontré tu proyecto por casualidad y me pareció perfecto para practicar.
¡Espero que sea de utilidad!
Agrego pruebas unitarias para el middleware ValidarSistema.
Summary by Sourcery
Add unit tests for the ValidarSistema middleware and adjust module dependencies.
Build:
Tests: