View Full Version : [CICLO 5 STORIA 2] Test Driven Development (Cover - TigerShark)
^TiGeRShArK^
16-11-2005, 19:07
Iniziamo questa sessione di TDD in pair programming.
Al solito NON COMMITTATE perchè lo stato del repository sarà precario dopo quello che faremo! :D
Ok, proviamo ^^
Anche aggiungendo noi la gemma, ci serve comunque un test che ci dica quando ne è presente un'altra subito sotto la posizione corrente del diamante. Noi sappiamo che è presente perchè la aggiungiamo tramite insertGemAt, però quando la gemma scende deve poter controllare se il blocco successivo sia libero per poter scendere ancora. Quindi il primo test è di aggiungere due gemme e controllare che sotto la prima ci sia la seconda. Ecco il test (speriamo in bene :rolleyes: ):
public void testGemInNextRow()
{
grid.insertGem(4, 3, gem1);
grid.insertGem(5, 3, gem2);
assertTrue("another gem in next row,in same column", grid.thereIsGemBelow(gem1));
}
Prima dovevo finire il 5.2.1 ;)
Ora lo committo...
Perchè i test nuovi o aggiunti non li mettete in it.diamonds.test.ignore ? Almeno la build machine non fa una build rossa...
Ancora un attimo epr il mio commit... Vi devo togliere il test che avete fatto...
^TiGeRShArK^
16-11-2005, 20:16
Perfetto.
il test fallisce correttamente :p
per farlo passare basta scrivere questo codice:
public boolean isGemBelow(Gem gem)
{
return true;
}
ho preferito chiamare il test isGemBelow() al posto di thereIsGemBelow() e ho aggiornato conseguentemente la classe TestGrid.
Ma ora posso anche pingermi un pochino avanti e controllare effettivamente se ci sia o meno una gemma nella casella sottostante....
per questo:
public boolean isGemBelow(Gem gem)
{
Cell currentCell = findGemCell(gem);
return isGemAt(currentCell.row() + 1, currentCell.column());
}
bene Build verde.....
ovviamente questo codice ancora "puzza" un pò.... ma dato che fa passare i test fa il suo compito e per ora può andare
Ragazzi, quando fate il commit di un test che fallisce, mettetelo in tests.ignore cosi' la build non si rompe.
Grazie :)
^TiGeRShArK^
16-11-2005, 20:25
si sistemato..
ho lasciato il test che passa in TestGrid ma ora ho creato TestGrid sotto ignore per poter lavorare tranquillamente là ;)
NON FATE COMMITTTTTTTT !!! :mad:
Ora potete lavor4arci, attenzione che ho rimosso il vostro codice...
Fek, mea culpa ^^
Ho modificato il test in questo modo:
public void testGemInNextRow()
{
grid.insertGem(4, 3, gem1);
assertFalse("Cell under current position is not empty", grid.isGemBelow(gem1));
grid.insertGem(5, 3, gem2);
assertTrue("Cell under current position is empty", grid.isGemBelow(gem1));
}
Per avere una certezza in più del suo funzionamento (ovviamente ora passa perchè hai già fatto la modifica prima)
^TiGeRShArK^
16-11-2005, 21:32
si in effetti mi sono fatto prendere un pò la mano :p
però mi sentivo abbastanza sicuro da scrivere 2 righe di codice (letterlamente :p) senza test.
Ora grazie a te abbiamo pure il test che ci assicura il corretto funzionamento del codice da me scritto ;)
Torniamo a noi...
A questo punto direi di verificare che la gemma continui la sua caduta se la cella sottostante è libera:
public void testGemIsMovingWhenNextCellIsEmpty()
{
grid.insertGem(4, 3, gem1);
grid.setGemUnderControl(gem1);
grid.setGravity(1.0f);
float oldPosY = gem1.getY();
grid.update();
assertEquals("Gem is not moving when cell under current position is empty", gem1.getY(), oldPosY + 1.0, 0.0000001);
}
il test passa dopo aver aggiunto la precisione.... suppongo sia un errore di arrotondamento.....
a te ;)
si in effetti mi sono fatto prendere un pò la mano :p
però mi sentivo abbastanza sicuro da scrivere 2 righe di codice (letterlamente :p) senza test.
Riprovaci e ti sego entrambe le mani :)
Si scrive nuovo codice solo in presenza di un test che fallisce.
^TiGeRShArK^
16-11-2005, 21:38
Riprovaci e ti sego entrambe le mani :)
Si scrive nuovo codice solo in presenza di un test che fallisce.
:asd:
mi è scappato kiedo venia! :p
cmq me ne sono reso conto subito e ho detto a cover d aggiungere il test corrispondente :D
(e ci aveva pensato pure lui contemporaneamente :D)
Bene, ora direi di controllare che la gemma non effettui alcun movimento in caso di una gemma presente nella riga sotto.
Test:
public void testGemIsStopWhenNextCellIsFull()
{
grid.insertGem(5, 2, gem1);
grid.insertGem(6, 2, gem2);
grid.setGemUnderControl(gem1);
grid.setGravity(1.0f);
grid.update();
assertTrue("Gem has overlapped one already present", gem1.notMoved);
}
Però sono indeciso per:
assertTrue("Gem has overlapped one already present", gem1.notMoved);
si potrebbe anche fare:
assertFalse("Gem has overlapped one already present", gem1.Moved);
Alla fine il concetto è uguale, però in questo caso qual'è meglio e più adatta? :confused:
Forse assertFalse, tu che dici ?
^TiGeRShArK^
16-11-2005, 22:08
personalmente preferirei l'assertFalse in combinazione con moved....
comunque ora scrivo il codice e vedo quale suona meglio per sicurezza :D
Ricordati di scrivere il minimo ed indispensabile per compilare il programma, ed in seguito per far passare il test, nulla di più.
Sempre se tieni alle mani :Prrr:
^TiGeRShArK^
16-11-2005, 22:18
Allora...
piccolo refactoring:
public void testGemIsMovingWhenLowerCellIsEmpty()
{
grid.insertGem(4, 3, gem1);
grid.setGemUnderControl(gem1);
grid.setGravity(1.0f);
float oldPosY = gem1.getY();
grid.update();
assertEquals("Gem is not moving when cell under current position is empty", gem1.getY(), oldPosY + 1.0, 0.0000001);
}
public void testGemIsStoppedWhenLowerCellIsFull()
{
grid.insertGem(5, 2, gem1);
grid.insertGem(6, 2, gem2);
grid.setGemUnderControl(gem1);
grid.setGravity(1.0f);
grid.update();
assertFalse("Gem has overlapped one already present", gem1.moved());
}
ho cambiato il nome dei test perchè credo che LowerCell renda meglio l'idea di NextCell e ho creato il codice minimo per farlo passare :D
public boolean moved()
{
return false;
}
avanti così che mi inizia a piacere! :D
Ma...ma...ma cosa vedo, che bello! Questo metodo non è male, ci serve anche per il test precedente, infatti possiamo usare direttamente .moved per controllare che effettivamente la gemma si sia mossa, senza utilizzare direttamente le coordinate nel test
Quindi cambierei il test precedente da:
public void testGemIsMovingWhenLowerCellIsEmpty()
{
grid.insertGem(4, 3, gem1);
grid.setGemUnderControl(gem1);
grid.setGravity(1.0f);
float oldPosY = gem1.getY();
grid.update();
assertEquals("Gem is not moving when cell under current position is empty", gem1.getY(), oldPosY + 1.0, 0.0000001);
}
A questa nuova versione:
public void testGemIsMovingWhenLowerCellIsEmpty()
{
grid.insertGem(4, 3, gem1);
grid.setGemUnderControl(gem1);
grid.setGravity(1.0f);
grid.update();
assertTrue("Gem is not moving when cell under current position is empty", gem1.moved());
}
^TiGeRShArK^
16-11-2005, 22:46
oh che bello!
il test appena modificato fallisce! :D
per farlo passare aggiungo:
private boolean moved;
public boolean moved()
{
return moved;
}
public void setMoved(boolean moved)
{
this.moved = moved;
}
e ovviamente setto moved a true quando viene spostata la gemma ovvero:
private void applyGravity(Gem gem, Cell cell)
{
if(gemHasCollidedWithBottom(gem))
{
final float heightFromGround = bounds.bottom() - gem.getHeight()
- gem.getY();
gem.move(0, heightFromGround);
if(!gem.wasDropped())
{
gem.drop();
}
}
else
{
gem.move(0, actualGravity);
gem.setMoved(true);
}
}
indovina un pò quale test fallisce ora? :D
Per la serie: Notte Horror, questa sera presentiamo in prima visione: "Code Horror", un film tratto da una storia vera :Prrr:
Non mi è venuto in mente niente a parte questa modifica ( :( ) di applyGravity:
private void applyGravity(Gem gem, Cell cell)
{
boolean isGemBelow = false;
if(!gemHasCollidedWithBottom(gem))
{
try
{
isGemBelow = isGemBelow(gem);
}
catch(IllegalArgumentException e)
{
}
}
if(gemHasCollidedWithBottom(gem) || isGemBelow)
:eekk:
Però i test passano tutti, il gioco funziona...anche se le gemme vengono disegnate in fondo le celle vengono occupate, da diamanti "fantasma" :Prrr:
^TiGeRShArK^
16-11-2005, 23:38
domani con calma limeremo anche questo codice horror :D
ora è tardi!
forza bimbi... su.... a nanna ke domani si lavora! :Prrr:
Sto seguendo Beck che nel suo libro dice di non andare nemmeno in bagno fin quando non si risolve in verde la build rossa. Non è lo stesso caso, perchè la build non è rossa, anzi, tutti i test passano. Però c'era quel codice, non mi convinceva..quindi ho provato a rifarlo, ma il risultato è stato (quasi) lo stesso...l'idea è sempre lì, non trovo altri modi :(
Modifiche fatte ora:
public void testIsGemBelowOverflow()
{
grid.insertGem(gridRows-1, 5, gem1);
assertFalse("Out of limits", grid.isGemBelow(gem1));
}
Ho aggiunto questo test per assicurarmi che non ci siano problemi in caso che si controlli la gemma quando si è già in fondo
Da questo ho cambiato qualcosa di isGemBelow diventa così:
public boolean isGemBelow(Gem gem)
{
Cell currentCell = findGemCell(gem);
if (currentCell.row < grid.length)
{
try
{
return isGemAt(currentCell.row() + 1, currentCell.column());
}
catch(IllegalArgumentException e)
{
}
}
return false;
}
Questo mi permette di avere un controllo maggiore, e di non cadere in eccezione quando la gemma arriva in fondo
Test passato.
Dopo sono tornato di nuovo all'altro test:
public void testGemIsStoppedWhenLowerCellIsFull()
{
grid.insertGem(5, 2, gem1);
grid.insertGem(6, 2, gem2);
grid.setGemUnderControl(gem1);
grid.setGravity(1.0f);
grid.update();
assertFalse("Gem has overlapped one already present", gem1.moved());
}
Questo non passa, va bene, allora torno all'idea precedente e modifico applyGravity in questo modo:
private void applyGravity(Gem gem, Cell cell)
{
if(gemHasCollidedWithBottom(gem) || isGemBelow(gem))
{
final float heightFromGround = bounds.bottom() - gem.getHeight()
- gem.getY();
gem.move(0, heightFromGround);
if(!gem.wasDropped())
{
gem.drop();
}
}
else
{
gem.move(0, actualGravity);
gem.setMoved(true);
}
}
Ok...anche questo test ora passa, provo a far partire il tutto...stessa cosa di prima, le gemme vengono inserite correttamente nella griglia, però vengono disegnate in fondo e non nella giusta posizione, inoltre "scatta" quando la gemma è a metà cella e non quando tocca l'altra.
Altra cosa che non mi piace assolutamente di questa implementazione è il fatto che comunque il test controlla .moved, e questo non viene mai impostato a false. Quindi l'implementazione del test imho non si può ritenere valida, anzi, è da rifare (che poi è da togliere solo una cosa). Domani comunque provo a pensare se trovo un altro modo per risolvere la cosa che ora sta diventando un pò troppo tardi (ho la sveglia alle 6).
Intanto da questa cosa, anche se sbagliata ho ricavato il test per isGemBelow, che non fa mai male ^^
In caso si accettano volentieri commenti/pareri/insulti (forse gli ultimi saranno maggiori, ho quest'impressione :rolleyes: ) ^^
Ok..ci siamo quasi... ^^
dato che quello fatto ieri non mi piaceva l'ho tolto:
private void applyGravity(Gem gem, Cell cell)
{
if(gemHasCollidedWithBottom(gem) || isGemBelow(gem))
{
final float heightFromGround = bounds.bottom() - gem.getHeight()
- gem.getY();
gem.move(0, heightFromGround);
if(!gem.wasDropped())
{
gem.drop();
}
}
else
{
gem.move(0, actualGravity);
gem.setMoved(true);
}
}
ora è tornato come prima(o meglio, prima ma senza riproduzione che ha postato cionci in update):
private void applyGravity(Gem gem, Cell cell)
{
if(gemHasCollidedWithBottom(gem))
{
final float heightFromGround = bounds.bottom() - gem.getHeight()
- gem.getY();
gem.move(0, heightFromGround);
}
else
{
gem.move(0, actualGravity);
gem.setMoved(true);
}
}
E il test non passa, va bene...allora cambio l'update in questo modo:
public void update()
{
if(getGemUnderControl() == null)
{
return;
}
moveGem(getGemUnderControl());
if(isGemBelow(gemUnderControl) || gemHasCollidedWithBottom(gemUnderControl))
{
gemUnderControl.setMoved(false);
if(!gemUnderControl.wasDropped())
{
gemUnderControl.drop();
}
insertNewGem();
}
}
Bene, build verde...
Faccio partire il tutto e....funziona in parte...c'è un piccolo problema (bug?) nell'aggiornamento della cella mentre il diamante si sta spostando, perchè aggiorna la cella quando arriva a metà e non quando il diamante la occupa tutta. Quindi in questo modo il diamante viene fermato a metà cella, però la soprapposizione funziona ^^
Ora provo a vedere se riesco a tirar fuori un test per questo problema, si accettano consigli :D
NOOOOOOOOOOOOOOOO, si è bloccato il pc e ho perso tutti i segnalibri di firefox(ho già guardato nella cartella del profilo ed è stato sovrascritto, quindi niente da fare :( )...sigh, quasi tutte cose che dovevo ancora leggere su XP, TDD, agile, etc... :cry: :cry: Vabbè.. google.. ^^
Comunque ho fatto anche un'altra modifica:
public boolean isGemBelow(Gem gem)
{
Cell currentCell = findGemCell(gem);
if (currentCell.row < grid.length)
{
try
{
return isGemAt(currentCell.row() + 1, currentCell.column());
}
catch(IllegalArgumentException e)
{
}
}
return false;
}
Visto che mi sembrava inutile lanciare un'eccezione per una cosa simile, controllo direttamente al suo interno che la gemma non sia già sul fondo (unico punto in cui viene lanciata un'eccezione perchè controlla con isGemAt se c'è una gemma sotto, e quindi esce dai limiti)
Quindi diventa così:
public boolean isGemBelow(Gem gem)
{
Cell currentCell = findGemCell(gem);
if (currentCell.row < grid.length - 1)
{
return isGemAt(currentCell.row() + 1, currentCell.column());
}
return false;
}
E tutto passa... ^^
^TiGeRShArK^
17-11-2005, 20:01
Perfetto!
io oggi ero impegnato, ma vedo che te la sei cavata egregiamente da solo ;)
direi TASK CONCLUSO! :D
Ho visto ora il bug... Fate un update che ho fatto alcuni cambiamenti...
Ditemi se il suono vi viene riprodotto o meno per ogni gemma (scrivetemelo nel thread apposito)...
Ora provo a vedere qualcosa per quel bug...comunque vi posso dire che prima non si verificava (la gemma arrivava alla fine della cella prima di finire in fondo alla griglia)...
Bè, se guardi la gemma (la prima) arriva in fondo alla cella quando deve toccare il fondo, il problema sono le successive, che succede a metà cella
Ho fatto un commit con un po' di refactoring. Ho spostato un po' di logica da Grid a Gem e fatto alcune semplificazioni.
Quel setMoved() non mi piace, "puzza" (:p), va trovata una soluzione migliore. Sono certo che se ne puo' fare a meno.
Ed io ho risolto il bug :)
Ed io ho risolto il bug :)
Bene :)
Che cos'era? Il test relativo?
In pratica ho cambiato anche i test... In un task precedente facevano cambiare cella ad una gemma quando la metà della gemma passava il bordo della cella...
Ora la gemma cambia cella quando è interamente dentro a quella nuova...
Poi ho leggermente cambiato il modo in cui veniva rilevata la collisione... Se viene rilevata una collisione alla gemma non viene applicata (giustamente) la gravità, come avveniva prima...
Inoltre c'era anche un piccolo bug... Se la gemma era accelerata e collideva non veniva fatto il movimento, ma la collisione era rilevata comunque, per questa devo mettere un test, me lo sono scordato, ma l'ho individuato mentre facevo le altre modifiche...
Ho individuato un bug che ho immesso cambiando quei test (avevo modificato erroneamente un test)... Ora cerco l'errore...
Inoltre c'era anche un piccolo bug... Se la gemma era accelerata e collideva non veniva fatto il movimento, ma la collisione era rilevata comunque, per questa devo mettere un test, me lo sono scordato, ma l'ho individuato mentre facevo le altre modifiche...
Ok, puoi aggiungere quel test?
Il test è questo:
public void testCorrectGemPositionAfterCollision()
{
float yStep = grid.getHeight() / 14;
grid.insertGem(6, 2, gem2);
grid.insertGem(4, 2, gem1);
grid.setGemUnderControl(gem1);
grid.setGravity(yStep - 1);
grid.update();
grid.update();
assertEquals("Gem is not in the correct position after update",
gem2.getY() - yStep, gem1.getY());
}
Preso. Ho fatto un altro po' di refactoring anch'io.
BlueDragon
17-11-2005, 23:14
Ho fatto il commit...
Ho lanciato il gioco, ottimo :)
Però abbiamo una cosa strana nei test:
public void testVerticalMove()
{
float yStep = grid.getHeight() / 14;
grid.setGravity((yStep / 2) + 1);
grid.insertGem(11, 4, gem);
grid.update();
assertTrue(!grid.isGemAt(11, 4) && grid.isGemAt(12, 4));
}
public void testVerticalMoveNotRow()
{
float yStep = grid.getHeight() / 14;
grid.setGravity(yStep - 1);
grid.insertGem(11, 4, gem);
grid.update();
assertFalse(grid.isGemAt(11, 4) && !grid.isGemAt(12, 4));
}
Se leggi gli assert, questi due test che sono stati modificati ora testano la stessa cosa....cioé che la gemma è passata da 11,4 a 12,4 sia con gravity "(yStep/2)+1" che con gravity "yStep-1".
In teoria il secondo test dovrebbe affermare che la gemma *non* è passata dalla row 11 alla row 12 se non è stata applicata abbastanza gravità.
Purtroppo però al momento basta 0.0001f di gravità per passare alla cella dopo e passare tutti e due i test..:)
E' corretto? Abbiamo deciso che per passare alla riga successiva ora basta entrarci con un decimo di millipixel?
E' corretto? Abbiamo deciso che per passare alla riga successiva ora basta entrarci con un decimo di millipixel?
No...ora li modifico...
Per passare alla riga successiva bisogna esserci completamente dentro...quindi il limite superiore della gemma deve essere all'interno della cella...
Grazie BlueDragon...c'era un errore :) Ora vediamo...
Avevo erroneamente fatto che la gemma passasse in una nuova cella appena la invadeva anche di un pixel...
Ora lo correggo in modo che venga messo in una nuova cella quando ci passa interamente...
Al limite potrebbe anche andare così... Voi che ne dite ?
Anzi, è più semplcie in questo modo !!! Ci si risparmia un nuovo metodo che fa lo stesso controllo di quello vecchio...
Mi sono preso la briga di decidere per tutti ed ho lasciato il codice in questo modo...
BlueDragon
18-11-2005, 00:31
Anzi, è più semplcie in questo modo !!! Ci si risparmia un nuovo metodo che fa lo stesso controllo di quello vecchio...
Mi sono preso la briga di decidere per tutti ed ho lasciato il codice in questo modo...
Se è più semplice va sempre bene :D
Però i test non sono ancora chiarissimi:
public void testVerticalMove()
{
float yStep = grid.getHeight() / 14;
grid.setGravity(yStep + 1);
grid.insertGem(11, 4, gem);
grid.update();
assertTrue(!grid.isGemAt(11, 4) && grid.isGemAt(12, 4));
}
public void testVerticalMoveNotRow()
{
grid.setGravity(1);
grid.insertGem(11, 4, gem);
grid.update();
grid.update();
assertTrue(grid.isGemAt(12, 4) && !grid.isGemAt(13, 4));
}
Nel primo: perché la gemma si muove solo da 11,4 a 12,4 e non fino a 13,4?
O meglio...io lo so perché si muove solo di 1 riga, ma in teoria visto che abbiamo stabilito che si passa di riga semplicemente invadendo la riga con un millipixel, il test non dovrebbe affermare che la gemma passa 2 righe avanti con una gravità di yStep+1?
Per quanto riguarda il secondo test...cosa testa adesso? Che con due update siamo passati di una row sola e non di due visto che la gravità è abbastanza da farci passare alla row 12 con il primo update ma non alla row 13 con il secondo update?
Forse è più chiaro se ci mettiamo anche un assertTrue(grid.isGemAt(12,4) && !grid.isGemAt(11,4)) tra i due update?
il primo test è sbagliato... La gemma non si deve muovere con una gravità superiore a yStep (magari va controlalto nel codice), altrimenti saltano i controlli sulla cella successiva...
Questo è coerente:
public void testVerticalMove()
{
grid.setGravity(1);
grid.insertGem(11, 4, gem);
grid.update();
assertTrue(!grid.isGemAt(11, 4) && grid.isGemAt(12, 4));
}
L'avevo inizialmente modificato, ma ho perso la modfica in un revert...
Per il secondo non importa quella assert perchè ci pensa già il primo test a verificarla...
La gemma non si deve muovere con una gravità superiore a yStep (magari va controlalto nel codice), altrimenti saltano i controlli sulla cella successiva...
Ah...questo già prima delle mie modifiche ;)
BlueDragon
18-11-2005, 00:49
L'avevo inizialmente modificato, ma ho perso la modfica in un revert...
Per il secondo non importa quella assert perchè ci pensa già il primo test a verificarla...
Questi revert malvagi mangia-codice... :D
E' vero che l'assert del secondo sarebbe su una cosa già verificata, ma forse rende più chiaro il test...o no? O forse lo rende più confuso proprio perché afferma una cosa che già si sa?
Sarei curioso di sapere dal nostro Coach se nei test bisogna mettere degli assert solo per ciò che si testa o se se ne possono mettere di altri per chiarire meglio i passaggi...
Questi revert malvagi mangia-codice... :D
E' vero che l'assert del secondo sarebbe su una cosa già verificata, ma forse rende più chiaro il test...o no? O forse lo rende più confuso proprio perché afferma una cosa che già si sa?
Sarei curioso di sapere dal nostro Coach se nei test bisogna mettere degli assert solo per ciò che si testa o se se ne possono mettere di altri per chiarire meglio i passaggi...
Per essere pignolissimi ci vorrebbe una sola assert per test. Ma preferisco privilegiare la chiarezza, quindi metti abbastanza assert affinche' il test sia chiaro.
Non si testano pero' condizioni gia' testate (e' una duplicazione).
vBulletin® v3.6.4, Copyright ©2000-2026, Jelsoft Enterprises Ltd.