added test for service #836
Conversation
|
|
||
| @BeforeEach | ||
| public void beforeEach() { | ||
| fruitTransaction = new FruitTransactionParser(); |
There was a problem hiding this comment.
Do we need to create a parser before each test?
|
|
||
| @Test | ||
| public void produceMapReportService_ZeroQuantity_Ok() { | ||
| Map<String, Integer> map = new LinkedHashMap<>(); |
There was a problem hiding this comment.
You always create a new map in every test, it will be better if you think about initialize it before all tests. Also, if you need to clear the data from the map, think about how you can achieve this.
|
Good job! But there are some points to consider. |
| @@ -119,4 +119,4 @@ | |||
| </plugins> | |||
| </pluginManagement> | |||
| </build> | |||
| </project> | |||
| </project> | |||
There was a problem hiding this comment.
Please don't commit this change, it's redundant
| public void fruitTransactionParser_OkCase() { | ||
| expected = List.of(new FruitTransaction(Operation.BALANCE, "banana", 20), | ||
| new FruitTransaction(Operation.BALANCE, "apple", 100), | ||
| new FruitTransaction(Operation.SUPPLY, "banana", 100), | ||
| new FruitTransaction(Operation.PURCHASE, "banana", 13), | ||
| new FruitTransaction(Operation.RETURN, "apple", 10), | ||
| new FruitTransaction(Operation.PURCHASE, "apple", 20), | ||
| new FruitTransaction(Operation.PURCHASE, "banana", 5), | ||
| new FruitTransaction(Operation.SUPPLY, "banana", 50)); | ||
| fileData = List.of("b,banana,20", | ||
| "b,apple,100", | ||
| "s,banana,100", | ||
| "p,banana,13", | ||
| "r,apple,10", | ||
| "p,apple,20", | ||
| "p,banana,5", | ||
| "s,banana,50"); | ||
| List<FruitTransaction> actual = fruitTransaction.parse(fileData); | ||
| Assert.assertEquals(expected, actual); | ||
| } | ||
|
|
||
| @Test | ||
| public void fruitTransactionParser_EmptyInput_NotOk() { | ||
| fileData = List.of(); | ||
| List<FruitTransaction> actual = fruitTransaction.parse(fileData); | ||
| assertTrue(actual.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| public void fruitTransactionParser_MalformedInput_NotOk() { | ||
| fileData = List.of("b,banana,20", "invalid_line", "s,apple,30"); | ||
| assertThrows(IllegalArgumentException.class, () -> fruitTransaction.parse(fileData)); | ||
| } | ||
|
|
||
| @Test | ||
| public void fruitTransactionParser_CaseInsensitiveOperationCode_Ok() { | ||
| fileData = List.of("B,banana,20", "S,apple,30"); | ||
| expected = List.of(new FruitTransaction(Operation.BALANCE, "banana", 20), | ||
| new FruitTransaction(Operation.SUPPLY, "apple", 30)); | ||
| List<FruitTransaction> actual = fruitTransaction.parse(fileData); | ||
| Assert.assertEquals(expected, actual); | ||
| } | ||
|
|
||
| @Test | ||
| public void fruitTransactionParser_InvalidQuantity_NotOk() { | ||
| fileData = List.of("b,banana,-20", "s,apple,invalid"); | ||
| assertThrows(IllegalArgumentException.class, () -> fruitTransaction.parse(fileData)); | ||
| } | ||
| } |
There was a problem hiding this comment.
| "p,banana,5", | ||
| "s,banana,50"); | ||
| List<FruitTransaction> actual = fruitTransaction.parse(fileData); | ||
| Assert.assertEquals(expected, actual); |
There was a problem hiding this comment.
Let's import this method as you did with assertTrue, assetThrows and so on
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class FruitTransactionParserTest { | ||
| private static Parser fruitTransaction; |
There was a problem hiding this comment.
Should this field be named as a fruitTransaction? It seems to store a parser
| expected = VALID_DATA; | ||
| map.put("banana", 152); | ||
| map.put("apple", 90); | ||
| String actual = produceMapReportService.produceReport(map); | ||
| assertEquals(expected, actual); |
There was a problem hiding this comment.
you can delete this expected field and just pass VALID_DATA directly into your assert methods, please check other places for this point
| } | ||
|
|
||
| @Test | ||
| public void produceMapReportService_ValidData_Ok() { |
There was a problem hiding this comment.
there is no need to include tested class name in each test name, please remove it everywhere
| @Test | ||
| void getStorage_ValidData_Ok() { | ||
| STORAGE.put("data1",1); | ||
| STORAGE.put("data2",2); | ||
| assertEquals(STORAGE,fruitDao.getStorage()); | ||
| } |
There was a problem hiding this comment.
I would say this method is redundant. We are not testing here any logic
| } | ||
|
|
||
| @Test | ||
| void add() { |
There was a problem hiding this comment.
Please, follow your previous naming conventions
| void validData_Ok() { | ||
| handler.handle(new FruitTransaction(Operation.BALANCE,"Banana",300)); | ||
| assertEquals(300, STORAGE.get("Banana")); | ||
| } | ||
|
|
||
| @Test | ||
| void get_InValidData_NotOk() { | ||
| assertThrows(IllegalArgumentException.class, | ||
| () -> handler.handle(new FruitTransaction(Operation.BALANCE, "Banana", -100))); | ||
| } |
There was a problem hiding this comment.
Please, be consistent with your test method names. Make sure your method names are consistent among all tests (handlers in this case)
No description provided.