Job response not read and closed: close localSender, Message, PipeLineSession too (#5324) #5340
Conversation
…d PipeLineSession too (#5324)
| finally { | ||
|
|
||
| localSender.open(); | ||
| localSender.sendMessageOrThrow(toSendMessage, session).close(); |
There was a problem hiding this comment.
Ik weet niet of het resultaat van sendMessageOrThrow een null zou kunnen zijn?
There was a problem hiding this comment.
Als het senden is mislukt, gooit hij een sendMessageOrThrow. In de andere gevallen is hij gevuld.
| } | ||
|
|
||
| @Test | ||
| void testHappyExecuteCall() throws JobExecutionException, TimeoutException, SenderException, IOException { |
There was a problem hiding this comment.
In unit tests hanteer ik tegenwoordig altijd "A A A" testing: expliciet aangeven van "Arrange", "Act", en "Assert".
In de test zet ik bovenaan een comment // Arrange, voor de regel(s) waar de te testen code wordt aangeroepen zet ik een comment // Act, en daarna wanneer ik de assertions ga doen dan een comment // Assert.
Dan is het sneller duidelijk waar in de test je moet kijken voor het uitvoeren van de eigenlijke code, etc.
Wanneer nodig dan herhaal ik de comments met bijvoorbeeld // Act 2, // Assert 2, etc.
Soms, bijvoorbeeld bij assertThrows, zijn "Act" en "Assert" in 1. Dan zet ik er een comment bij // Act / Assert.
There was a problem hiding this comment.
Dat kende ik nog niet. De structuur die ik meestal aanhoud is:
- een lege regel tussen de validatie en uitvoer vd method
- een beschrijvende test-methode naam kiezen, waaruit blijkt waarom de test bestaat
- bij meerdere unit tests de setup zaken die overlappen naar een @beforeeach blok verplaatsen
- comments toevoegen als het groter wordt en niet direct duidelijk wat er gebeurt. Uiteraard is dat goed om te toetsen door een reviewer.
Ik wil iig voorkomen dat de comments een herhaling van de code is wat er al staat. Dat voegt dan niets toe. Het zou hoogstens het waarom moeten toevoegen of context geven.
Ik zal eens kijken of ik jouw methodiek kan toepassen bij grotere unit tests een volgende keer. Oke?
There was a problem hiding this comment.
Beschrijvende method naam een gebruik van setup methods zijn natuurlijk altijd wenselijk!
De comments // Arrange, // Act, // Assert zijn extra, om in de bestaande test code het onderscheid aan te geven.
Makkelijk toe te voegen zonder de tests te herschrijven, en maken de tests leesbaarder omdat je sneller de test-code kan scannen voor de comments. :-)
There was a problem hiding this comment.
Oke thanks voor de tips! Ik zal het toepassen als de testen groter worden. 👍
|
|
|
I don't understand the Codecov results.. It associates lots of |
|
Kudos, SonarCloud Quality Gate passed!
|
We have that problem too. That's why I suggested merging master into your branch and seeing if that cleans up those codecov results. |
| //sendMessage message cannot be NULL | ||
| Message message = new Message((getMessage()==null) ? "" : getMessage()); | ||
| PipeLineSession session = new PipeLineSession(); | ||
| try (Message toSendMessage = new Message((getMessage() == null) ? "" : getMessage()); |
There was a problem hiding this comment.
Misschien dat het een verbetering is als we een NullMessage zouden gebruiken in plaats van een leeg message.
Ik denk voor nu even buiten scope, maar wel een TODO waardig
There was a problem hiding this comment.
Yes prima idee. Neem ik mee in mijn volgende PR, over hetzelfde onderwerp ;-)
Die komt er aan.
(cherry picked from commit 6901bf9)
Closing multiple objects when possible. Add unit testing, was partly covered by other unit tests already. Mostly the configure method.