-
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
implement test for fruit shop coverage 97% #776
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.
Good job! Let's improve your solution!
|
||
class PurchaseOperationHandlerTest { | ||
private final Fruit apple = new Fruit("apple"); | ||
private final OperationHandler purchaseOperationHandler = new PurchaseOperationHandler(); |
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.
It's better to use BeforeAll annotation.
Check all places
import org.junit.jupiter.api.Test; | ||
|
||
class FruitTest { | ||
@Test |
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.
This test class is redundant
|
||
@Test | ||
void emptyStorageValidReport_Ok() { | ||
Storage.storage.clear(); |
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.
It's better to use AfterEach annotation to clear the storage.
Check all places
|
||
@Test | ||
public void parse_validData_returnsParsedTransactions() { | ||
|
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.
Remove empty line
inputData.add("INVALID_DATA"); | ||
Assert.assertThrows(ArrayIndexOutOfBoundsException.class, | ||
() -> transactionParserService.parse(inputData)); | ||
} |
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.
It's better to add test methods for each invalid case like: invalid operation type, invalid field amount and invalid quantity value
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.
I have ReaderValidationImpl class which skip for me all invalid operation in input file and all incorrect operations do not fall into this class +all of test cases of invalid input in ReaderValidationImplTest
balanceOperationHandler = new BalanceOperationHandler(); | ||
} | ||
|
||
@BeforeEach |
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 before? same everywhere
import org.junit.jupiter.api.Test; | ||
|
||
class ReaderValidationImplTest { | ||
private final ReaderValidation readerValidation = new ReaderValidationImpl(); |
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.
let's create text data in tests where they are actually needed. Same in all test classes
class OutputValidatorImplTest { | ||
private final String invalidFileForTest = "OutputValidatorinValidTest.csv"; | ||
private final String invalidQuantityFile = "resources/FileWithInvalidQuantity.csv"; | ||
private final OutputValidatorImpl outputValidator = new OutputValidatorImpl(); |
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 do you need final variables and let's initialize the services in the BeforeAll methods. Same in all test classes
private final OutputValidatorImpl outputValidator = new OutputValidatorImpl(); | ||
|
||
@Test | ||
void fileValid_Ok() { |
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.
which method are you testing here?) Let's follow the rules for naming test methods. Likewise in other cases
|
||
@Test | ||
void fileValid_Ok() { | ||
String validFileForTest = "src/test/java/resources/OutputValidatorValidTest.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.
why are some variables class fields, others method fields?
void invalidQuantity_NotOk() { | ||
Assert.assertThrows(RuntimeException.class, | ||
() -> outputValidator.validateFile(invalidQuantityFile)); | ||
} |
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.
how about test with null parametr?
class BalanceOperationHandlerTest { | ||
private static Fruit apple; | ||
private static OperationHandler balanceOperationHandler; | ||
private final FruitTransaction fruitTransaction = new FruitTransaction(); |
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 some fields are initialized immediately and some in the Before All method?
Integer expected = 33; | ||
Integer actual = Storage.storage.get(apple); | ||
Assertions.assertEquals(expected, actual); | ||
} |
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.
add a test so that there is already some of the fruit in Storage and they will deliver more
private static final Fruit banana = new Fruit("banana"); | ||
private static final Fruit apple = new Fruit("apple"); | ||
private static int initialBananaBalance; | ||
private static int initialAppleBalance; |
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 do you need these variables as class fields
.append(System.lineSeparator()) | ||
.toString(); | ||
Assertions.assertEquals(expected,report); | ||
} |
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.
add test for empty storage
@BeforeEach | ||
void setUp() { | ||
reportService = new ReportServiceImpl(); | ||
Storage.storage.clear(); |
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 you need this
|
||
class ReaderServiceImplTest { | ||
private final String nonExistFile = "nonExistFile.csv"; | ||
private final ReaderService readerService = new ReaderServiceImpl(); |
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.
lets initializate it in beforeAll nethod
private final List<String> emptyList = new ArrayList<>(); | ||
private final List<String> invalidFirstLine = new ArrayList<>(List.of( | ||
"type,fruit,quantit", | ||
"b,banana,20", | ||
"s,apple,20" | ||
)); | ||
private final List<String> validStringList = new ArrayList<>(List.of( | ||
"type,fruit,quantity", | ||
"b,banana,20", | ||
"s,apple,35" | ||
)); | ||
private final List<String> invalidQuantityList = new ArrayList<>(List.of( | ||
"type,fruit,quantity", | ||
"b,banana,-10", | ||
"b,banana,20", | ||
"s,apple,35" | ||
)); | ||
private final List<String> invalidOperationList = new ArrayList<>(List.of( | ||
"type,fruit,quantity", | ||
"f,banana,10", | ||
"b,banana,20", | ||
"s,apple,35" | ||
)); |
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.
not fixed. let's create test data in tests where they are actually needed
} | ||
|
||
@Test | ||
void correctSkipInvalidQuantity_Ok() { |
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.
let's create text data in tests where they are actually needed
No description provided.