Qa 5637 refactor classe miglioramento classe invio notifiche b2b steps #667
Conversation
…se-Miglioramento-classe-InvioNotificheB2bSteps
pom.xml
Outdated
There was a problem hiding this comment.
I cambiamenti avvenuti in
- pom.xml
- src/main/java/it/pagopa/pn/client/b2b/pa/service/IPnPaymentInfoClient.java
- src/main/java/it/pagopa/pn/client/b2b/pa/service/impl/PnPaymentInfoClientImpl.java
- src/test/java/it/pagopa/pn/cucumber/steps/pa/AsyncSteps.java
sono pertinenti al ticket?
There was a problem hiding this comment.
(le osservazioni fatte su questo file sono estensibili a tutto ciò che riguarda i test)
|
|
||
| PnPollingResponseV26 pnPollingResponseV26 = statusRapidV26.waitForEvent(sharedSteps.getSentNotification().getIun(), | ||
| // Prevenzione NullPointerException |
There was a problem hiding this comment.
Trasmette un messaggio chiaro e diretto mantenendo il messaggio di errore sottostante, con due appunti:
- Questi messaggi di errore sono diretti al futuro analista, che nella normalità dei casi non sa come è fatto il test e che dunque potrebbe beneficiare di un'informazione a più alto livello, visto anche che quello a basso livello continua a essere stampato da AssertJ (poiché la clausola 'as', giustamente, non lo sovrascrive): cosa vuol dire che
sharedSteps.getSentNotification()risulta null? Vuol dire che non è stata trovata una notifica a cui far riferimento nel contesto dei test. Questo è cosa è successo, e potrebbe costituire la prima parte del messaggio; nella seconda si può provare a corredare di ipotesi sul perché, per tentare di dare una prima direzione all'analista. Come mai non c'è una notifica nel contesto? Magari perché il set della notifica nel contesto non è avvenuto correttamente in uno degli step precedenti. - Notiamo le ragioni di questo errore non dipendono dalla funzionalità che stiamo testando, ma dal test stesso. Riterrei dunque che non dovrebbe essere lanciata un assertion, ma un eccezione. Se un assertion fallisce, il messaggio è "il test ha funzionato correttamente ma il suo esito è stato negativo". Qui il test non ha funzionato bene, si è inceppato, e dunque l'informazione da trasmettere riterrei debba essere "il test non ha funzionato perché si è rotto, occorre una revisione", appunto con il lancio di un'eccezione.
| ); | ||
|
|
||
| // Prevenzione NullPointerException | ||
| assertThat(pnPollingResponseV26) |
There was a problem hiding this comment.
Discorso simile al punto precedente: non è previsto dal corretto flusso del test che quel risultato possa essere null, riterrei si debba quindi lanciare un'eccezione corredata da un messaggio di errore che esprima cosa è successo e sulle possibili cause (se si fa fatica a capire la regione per cui quella chiamata possa mai restituire NULL, ci si potrebbe accontentare affermando che non è previsto che questa possa restituire null)
| .as("La risposta di polling non dovrebbe essere nulla") | ||
| .isNotNull(); | ||
|
|
||
| assertThat(pnPollingResponseV26.getNotification()) |
There was a problem hiding this comment.
Discorso simile al punto precedente, andrebbero indagare le ragioni per cui possa mai essere NULL e lanciare un eccezione
| .as("La notifica nella risposta di polling non dovrebbe essere nulla") | ||
| .isNotNull(); | ||
|
|
||
| assertThat(pnPollingResponseV26.getNotification().getNotificationStatusHistory()) |
There was a problem hiding this comment.
Siamo sicuri che che il risultato non debba essere nullo? Se si tratta di un semplice check a beneficio della log, volendo si potrebbe anche fare a meno di questo check
| try { | ||
| Assertions.assertTrue(pnPollingResponseV26.getResult()); | ||
| Assertions.assertNotNull(pnPollingResponseV26.getNotificationStatusHistoryElement()); | ||
| assertThat(pnPollingResponseV26.getResult()) |
There was a problem hiding this comment.
Discorso simile al primo punto sollevato: cosa significherebbe che il risultato sia false? Che è stato raggiunto un polling timeout. Possibile messaggio: "L'elemento di timeline cercato non è stato rilevato dopo {int} tentativi separati ciascuno da un intervallo di {int} secondi, è possibile che non sia arrivato o che sia arrivato in ritardo."
Visto che è una condizione di errore che verosimilmente non dipenderebbe dal test la tratterei come assertion, diversamente dai casi precedenti.
| try { | ||
| Assertions.assertTrue(pnPollingResponseV26.getResult(), "Polling failed. IUN: " + sharedSteps.getSentNotification().getIun()); | ||
| Assertions.assertNotNull(pnPollingResponseV26.getTimelineElement(), "The timeline element was not found. IUN: " + sharedSteps.getSentNotification().getIun()); | ||
| assertSoftly(softly -> { |
There was a problem hiding this comment.
Uso tecnicamente corretto delle soft assertions, tuttavia se il polling non ha avuto successo allora non sarà presente un elemento di timeline, dunque in questo caso sospetto se ne possa fare a meno
EDIT: sorge il dubbio sul perché fare il secondo controllo a prescindere, se teoricamente basta il primo
…sse-InvioNotificheB2bSteps
…sse-InvioNotificheB2bSteps
…sse-InvioNotificheB2bSteps
QA-5637