-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Solution for jv-fruit-shop-test #838
base: main
Are you sure you want to change the base?
Conversation
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.
Overall good job but i would recommend to make magic numbers constants and delete test file report after tests) Moreover, u can put AfterAll and AfterEach to the end for better readability
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.
Good, but some points should be considered. Pay attention to the constants, there are a lot of places, where it would be beneficial to use them
void processTransactions_validTransactions_updatesStorage() { | ||
FruitTransaction fruitTransaction = new FruitTransaction(); | ||
fruitTransaction.setOperation(FruitTransactionOperation.BALANCE); | ||
fruitTransaction.setFruit("banana"); |
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.
My recommendation here is to move "banana" literal to the constant variable
} | ||
|
||
@Test | ||
public void testInvalidStringsToFruitTransactions() { |
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.
Make sure your test method names are consistent among all tests
|
||
@Test | ||
public void readFromFile_validFile_ok() { | ||
String validFilePath = "src/test/resources/test_transaction.csv"; |
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.
Usually we are extracting path values to the constants. That is also a general recommendation for your code optimization
Storage.fruitTransactions.put("apple",300); | ||
String expected = "fruit,quantity" + separator | ||
+ "banana,100" + separator + "apple,300" + separator; | ||
assertEquals(expected, |
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.
Call to assertEquals()
from Assert
should be replaced with a call to a method from org.junit.jupiter.api.Assertions
for consistency sake
void writeToFile_validPath_ok() { | ||
String report = "fruit,quantity" + System.lineSeparator() | ||
+ "banana,152" + System.lineSeparator() | ||
+ "apple,90" + System.lineSeparator(); | ||
String validFilePath = "test_valid_report.csv"; | ||
writerService.writeToFile(validFilePath,report); | ||
Path path = Paths.get(validFilePath); | ||
List<String> strings = null; | ||
try (Stream<String> lines = Files.lines(path)) { | ||
strings = lines.collect(Collectors.toList()); | ||
} catch (IOException e) { | ||
fail("Exсeption shouldn't be thrown in this test"); | ||
} | ||
String[] transactions = report.split(System.lineSeparator()); | ||
for (int i = 0; i < strings.size(); i++) { | ||
String expected = strings.get(i); | ||
assertEquals(expected,transactions[i], | ||
"transactions differ at element: " + i); | ||
} | ||
} |
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.
Looks too overcomplicated. Consider making it prettier with assertIterableEquals
and some some values to constant separation.
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.
A few points to consider
public void setUp() { | ||
parserService = new ParserServiceImpl(); | ||
} | ||
|
||
@Test | ||
public void testValidStringsToFruitTransactions() { | ||
List<String> inputStrings = Arrays.asList( | ||
BALANCE_OPERATION_CODE + ",apple,10", | ||
SUPPLY_OPERATION_CODE + ",banana,20", | ||
PURCHASE_OPERATION_CODE + ",orange,5", | ||
RETURN_OPERATION_CODE + ",kiwi,3" | ||
); | ||
|
||
List<FruitTransaction> expectedTransactions = Arrays.asList( | ||
createFruitTransaction(FruitTransactionOperation.BALANCE, "apple", 10), | ||
createFruitTransaction(FruitTransactionOperation.SUPPLY, "banana", 20), | ||
createFruitTransaction(FruitTransactionOperation.PURCHASE, "orange", 5), | ||
createFruitTransaction(FruitTransactionOperation.RETURN, "kiwi", 3) | ||
); | ||
|
||
List<FruitTransaction> result = parserService.stringsToFruitTransactions(inputStrings); | ||
|
||
assertEquals(expectedTransactions, result); | ||
} | ||
|
||
@Test | ||
public void testInvalidStringToFruitTransactions() { | ||
List<String> inputStrings = Arrays.asList( | ||
BALANCE_OPERATION_CODE + ",apple,10", |
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.
try { | ||
operationHandler.countQuantity(INITIAL_AMOUNT, NEGATIVE_OPERATION_AMOUNT); | ||
} catch (RuntimeException e) { | ||
String expected = "Operation amount can not be less than 0"; | ||
assertEquals(expected, e.getMessage()); | ||
} | ||
} | ||
|
||
@Test | ||
void countQuantity_negativeCurrentAmount() { | ||
try { | ||
operationHandler.countQuantity(NEGATIVE_CURRENT_AMOUNT, OPERATION_AMOUNT); | ||
} catch (RuntimeException e) { | ||
String expected = "Current amount can not be less than 0"; | ||
assertEquals(expected, e.getMessage()); | ||
} |
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.
Why don't you use assertThrows() here?
No description provided.