-
Notifications
You must be signed in to change notification settings - Fork 57
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
agrega test en ui #20
base: master
Are you sure you want to change the base?
Conversation
src/ui/__tests__/listado.spec.js
Outdated
let pokemones = document.querySelectorAll('a.list-group-item.list-group-item-action') | ||
expect(pokemones).toHaveLength(20); | ||
|
||
pokemones[4].click(); |
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.
este 4 es por algo en particular?
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.
Puse 4 para que haga click en esa página, pensando que si o si debía ser una en particular. Me toma si pongo pokemones.click() ?
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, tiene que ser una página particular, sólo me llamó la atención que sea la nro 4 por algún motivo. Capaz con viene que hagas un random de 0 a 19, pero si no está bien así, no es nada mayor.
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.
ahí cambié por esto:
const numeroAleatorio = Math.floor(Math.random() * 20);
pokemones[numeroAleatorio].click();
Así me genera el numero al azar
src/ui/__tests__/paginador.spec.js
Outdated
|
||
let paginador = document.querySelector('#paginador'); | ||
let totalItemsPaginador = paginador.querySelectorAll('li'); | ||
let totalPaginas = Math.ceil(1118/20) + 2; |
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.
estos son números mágicos, escribí en 3 variables separadas qué representa 1118, 20 y 2.
src/ui/__tests__/paginador.spec.js
Outdated
let paginador = document.querySelector('#paginador'); | ||
let totalItemsPaginador = paginador.querySelectorAll('li'); | ||
let totalPaginas = Math.ceil(1118/20) + 2; |
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 regla, ponele const
a todo, sólo ponele let a las cosas que vas a reasignar.
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 lo cambio
mockCallback | ||
); | ||
|
||
let pagina = document.querySelector("#paginador > li:nth-child(5) > a"); |
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.
para no depender de la estructura de HTML, deberías ponerle una clase a lo que querés clickear, así tu selector queda algo como #paginador .pagina
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.
const anterior = document.querySelector('#paginador > li.page-item.disabled')
expect(anterior.classList.contains('disabled')).toBe(true);
const siguiente = document.querySelector("#paginador > li.page-item.disabled");
expect(siguiente.classList.contains('disabled')).toBe(true);
Esto hice porque no sé como encarar el click y saber que fue a un elemento disabled. Cambié a que me diga si tiene la clase y me devuelva true
src/ui/__tests__/paginador.spec.js
Outdated
let anterior = document.querySelector('#paginador > li.page-item.disabled'); | ||
expect(anterior.textContent).toEqual('Anterior'); | ||
expect(anterior.classList).toContain('disabled'); |
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.
lo de testear que el texto sea "Anterior" está bien, pero una mejor prueba sería que le hagas click y que no pase nada, si tiene la clase disabled o no, no es importante.
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.
Esto lo hice desde el absurdo porque no supe que hacer al clickearlos... no vi cómo podía saber si al clickear se podía ejecutar o no, solo vi el href en null pero preferí ir por esto
expect(document.querySelector('#pokemon-id').textContent) | ||
.toEqual('1'); | ||
|
||
let tipos = document.querySelectorAll('#tipos span'); |
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.
mismo acá #tipos .tipo
, tu test no se debería romper sólo porque ahora en vez de un querés motrar un
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 quise cambiarte los className pero si me parece mala práctica seleccionarlos así.
src/ui/paginador.js
Outdated
@@ -15,6 +15,7 @@ function crearItemPaginador(texto, url = '#') { | |||
export function manejarCambioPagina(e, callbackPaginaSeleccionada = () => {}) { | |||
e.preventDefault(); | |||
const { target } = e; | |||
console.log(target.dataset); |
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.
esto si no es necesario, volalo
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.
si perdón lo puse para saber bien cuál era ver cómo traía el dato, ya lo borro
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á bien, te marco algunos cambios chicos para hacer para que los tests sean más legibles
No description provided.