-
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
add tests to fruitshop solution #787
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.
LGTM! Actually, I don't know what to fix here, structure looks good.
void addProductToEmptyStorage_Ok() { | ||
StorageDao storageDao = new StorageDaoImpl(); | ||
storageDao.add(banana, quantity); | ||
Integer expected = 50; |
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.
maybe you should use constants for all the numbers, so code could look cleaner
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.
you don't need to use constants in tests
static void setUp() { | ||
apple = new Fruit("apple"); | ||
supplyOperationHandler = new SupplyOperationHandler(); | ||
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.
Should you use Storage.storage.clear()
here if you use it in @AfterEach?
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.
agree with @validatorr
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.
pay attention to your method names, they don't match the pattern
private final Fruit banana = new Fruit("banana"); | ||
private final Fruit apple = new Fruit("apple"); | ||
private final int quantity = 50; |
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 u need this field?
void addProductToEmptyStorage_Ok() { | ||
StorageDao storageDao = new StorageDaoImpl(); | ||
storageDao.add(banana, quantity); | ||
Integer expected = 50; |
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.
you don't need to use constants in tests
|
||
@Test | ||
void addProductToEmptyStorage_Ok() { | ||
StorageDao storageDao = new StorageDaoImpl(); |
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 it class field and use in all test methods
} | ||
|
||
@Test | ||
void subtractProduct_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.
wrong name. which of the methods are you testing. Same in another tests
private static Fruit apple; | ||
private static OperationHandler balanceOperationHandler; | ||
private static FruitTransaction 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.
private static Fruit apple; | |
private static OperationHandler balanceOperationHandler; | |
private static FruitTransaction fruitTransaction; | |
private static OperationHandler balanceOperationHandler; |
u dont need this as class field
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.
same in another classes
@BeforeAll | ||
static void setUp() { | ||
apple = new Fruit("apple"); | ||
balanceOperationHandler = new BalanceOperationHandler(); |
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.
StorageDao in your implementations must be initialized in the constructor. Same everywhere
static void setUp() { | ||
apple = new Fruit("apple"); | ||
supplyOperationHandler = new SupplyOperationHandler(); | ||
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.
agree with @validatorr
final int initialBananaBalance = Storage.storage.get(banana); | ||
final int initialAppleBalance = Storage.storage.get(apple); | ||
int quantity = 25; | ||
FruitTransaction transaction1 = 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.
StorageDao in your implementations must be initialized in the constructor. Same in another cases
transactions.add(transaction1); | ||
transactions.add(transaction2); | ||
|
||
operationProcessorService.process(transactions, operationStrategy); |
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.
OperationStrategy must by class field. U dont need set it as parameter
No description provided.