Skip to content
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

Write tests with more than 80% coverage #834

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DEz-1996
Copy link

@DEz-1996 DEz-1996 commented Nov 3, 2023

No description provided.


@Test
void listOperationsFromCsv_validText_ok() {
List<String> inputList = List.of("b,banana,20", "b,apple,100");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to create String of operation before each method or like constant, and use in each testMethods and in that way avoid code duplication

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's a good point

balanceOperation = new GoodsOperation(balanceOperation.getTransactionType(),
null,
balanceOperation.getQuantity());
Assertions.assertThrows(RuntimeException.class,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to throw more specific exception or check exception message like example bellow:

@test
public void whenExceptionThrown_thenAssertionSucceeds() {
Exception exception = assertThrows(NumberFormatException.class, () -> {
Integer.parseInt("1a");
});

String expectedMessage = "For input string";
String actualMessage = exception.getMessage();

assertTrue(actualMessage.contains(expectedMessage));

}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool advice! Thank you!

purchaseOperation = new GoodsOperation(purchaseOperation.getTransactionType(),
null,
purchaseOperation.getQuantity());
Assertions.assertThrows(RuntimeException.class,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to throw more specific exception or check exception message

returnOperation = new GoodsOperation(returnOperation.getTransactionType(),
null,
returnOperation.getQuantity());
Assertions.assertThrows(RuntimeException.class,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to throw more specific exception or check exception message

supplyOperation = new GoodsOperation(supplyOperation.getTransactionType(),
null,
supplyOperation.getQuantity());
Assertions.assertThrows(RuntimeException.class,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to throw more specific exception or check exception message

Copy link

@kshuryhin kshuryhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, consider some points to improve

Comment on lines 24 to 26
@AfterEach
void tearDown() {
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if we really need that one if it does nothing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to remove it...

Comment on lines 35 to 37
for (int i = 0; i < actual.size(); i++) {
Assertions.assertEquals(expected.get(i), actual.get(i));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int i = 0; i < actual.size(); i++) {
Assertions.assertEquals(expected.get(i), actual.get(i));
}
assertIterableEquals(expected, actual);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool advice! Thank you!

Comment on lines 53 to 55
for (int i = 0; i < expectedList.size(); i++) {
Assertions.assertEquals(expectedList.get(i), actualList.get(i));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int i = 0; i < expectedList.size(); i++) {
Assertions.assertEquals(expectedList.get(i), actualList.get(i));
}
assertIterableEquals(expectedList, actualList);

Comment on lines 64 to 66
for (int i = 0; i < actualList.size(); i++) {
Assertions.assertEquals(expectedList.get(i), actualList.get(i));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int i = 0; i < actualList.size(); i++) {
Assertions.assertEquals(expectedList.get(i), actualList.get(i));
}
assertIterableEquals(expectedList, actualList);

Comment on lines 43 to 45
for (int i = 0; i < actualList.size(); i++) {
Assertions.assertEquals(expectedsList.get(i), actualList.get(i));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int i = 0; i < actualList.size(); i++) {
Assertions.assertEquals(expectedsList.get(i), actualList.get(i));
}
assertIterableEquals(expectedsList, actualList);

Copy link

@kshuryhin kshuryhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants