Task complete

[Hubris0]

Prepared test coverage for creating transaction report for supplied transactions.

[slade13]

You have already written several useful scenarios and you are thinking about invalid input, insufficient fruit quantity, and the complete application flow, which is a good start. However, the most important requirement of this task is currently missing: unit tests must be isolated.

Right now almost the whole application is tested through one MainTest, with multiple services, strategies, file operations, and handlers participating in the same tests. Split these tests into separate classes and test every component directly. Prepare Storage directly when testing transactions or report creation instead of reaching the tested functionality through other services.

Also improve test naming so that the first part contains the actual method under test, remove assertions that only verify the manually prepared input, and move test files to src/test/resources.

[slade13]

Tests should be separated by responsibility, currently everything is placed inside:

class MainTest

This is directly against the task requirements. You should create separate test classes for the components you want to verify, for example:

ReaderImplTest
WriterImplTest
ReportCreatorTest

BalanceTransactionTest
SupplyTransactionTest
PurchaseTransactionTest
ReturnTransactionTest

OperationStrategyImplTest
TransactionHandlerTest

Creating MainTest still suggests that you're testing the application's complete flow rather than individual units.

[slade13]

Please review my comments, additionally fields like:

private static final String fruitName
private static final int fruitQuantity

are constants and should normally use uppercase naming:

private static final String FRUIT_NAME = "testfruit";
private static final int FRUIT_QUANTITY = 123;

Please fix things mentioned by my comments and request another review.

[slade13]

You have:

private final ReturnTransaction returnTransaction = new ReturnTransaction();

and later:

returnTransaction.process(fruitName, suppliedQuantity);

So despite the class being named SupplyTransactionTest, it actually tests ReturnTransaction.

[slade13]

This test still combines:

• ReaderImpl
• TransactionHandler
• OperationStrategyImpl
• all transaction implementations
• ReportCreator
• WriterImpl
• another ReaderImpl

This recreates almost the entire program flow.

The task explicitly says:

Keep your strategy, handler, and service tests separate from each other.

For TransactionHandlerTest, it is enough to create a strategy containing only the transaction needed for the particular test.

[slade13]

The task explicitly says that the test resources should be placed in:

src/test/resources/[your-files.csv]

but the project still contains test files such as:

src/main/resources/testOk.csv
src/main/resources/testEmpty.csv
src/main/resources/testIncorrectAmount.csv
src/main/resources/testMissingElement.csv

These should be moved to:

src/test/resources

[slade13]

This test file currently checks only:

assertTrue(Files.exists(Path.of(path)));

That verifies that a file exists, but not whether WriterImpl actually wrote the correct data.

A stronger test should also read the file and compare its contents.

[slade13]

The test expects:

assertThrows(...)

So this is clearly a negative scenario and should be:

Suggested change

	void process_purchaseMoreThanAvailable_ok() {
	void process_purchaseMoreThanAvailable_notOk() {

[slade13]

Same here:

Suggested change

	void process_fruitNotExist_ok() {
	void process_fruitNotExist_notOk() {

[slade13]

OperationStrategyImplTest can be simpler and more isolated, currently you create all real transaction implementations:

Map.of(
    "b", new BalanceTransaction(),
    "s", new SupplyTransaction(),
    "p", new PurchaseTransaction(),
    "r", new ReturnTransaction()
)

To test OperationStrategyImpl, you don't really need to verify every concrete transaction class.

The important behavior is:

• existing operation returns the corresponding handler;
• unsupported operation throws an exception

A map containing one transaction is enough for this unit test and follows the task hint:

It's enough to create a map with only one handler.

So something like:

Map.of("b", new BalanceTransaction())

would be sufficient.

[mateuszwojtkowiak]

Good job! Please read my comments and apply changes.

[mateuszwojtkowiak]

Tests should be isolated and path is not constant, so it would be better to use local variables:

void readFromFile_noFile_notOk() {
        String path = "";

[mateuszwojtkowiak]

The message "File to read should not have been found" is grammatically incorrect and unnatural.
It suggests that the file was actively searched for and should not have been found, which is not the intended meaning. In this test, we simply expect that the file does not exist.
A clearer and idiomatic message would be: "File to read could not be found" or "Expected the file to be missing".

[mateuszwojtkowiak]

If file exists, then this message should not say that it is empty. It should look like that:

Expected file should contain data.

[mateuszwojtkowiak]

The test expects that file will be empty. But the message says that it is not empty.
It should look like that:

Expected file to be empty.

[mateuszwojtkowiak]

You expect that storage should not be empty, but the message says that it is not. The message should look like that:

Expected storage to be empty.

[mateuszwojtkowiak]

get() on Optional can give you NoSuchElementException so it's better to use AssertionError with descriptive message:

Fruit fruit = fruits.stream()
        .filter(f -> f.getName().equals(FRUIT_NAME))
        .findFirst()
        .orElseThrow(() -> new AssertionError("Fruit was not added to storage"));

[mateuszwojtkowiak]

You don't need loop because you test only one line:

assertThrows(ArrayIndexOutOfBoundsException.class,
        () -> transactionHandler.process("b,banana"));

[mateuszwojtkowiak]

You don't need loop because you test only one line:

assertThrows(ArrayIndexOutOfBoundsException.class,
        () -> transactionHandler.process("b,banana,10,extra"));

[mateuszwojtkowiak]

You don't need loop because you test only one line:

assertThrows(ArrayIndexOutOfBoundsException.class,
        () -> transactionHandler.process("b,banana,-2")));

[mateuszwojtkowiak]

For tests that use this method, it's worth adding assertion that will check if Storage has exactly one fruit.

assertEquals(1, Storage.getFruits().size());

[slade13]

Good job, your code is much closer to proper unit testing and the major structural problems from the previous submissions have been fixed.

The main remaining improvement is isolation: WriterImplTest should not use ReaderImpl, and TransactionHandlerTest doesn't need to retest all four transaction implementations because those already have dedicated tests.

Please review my comments and update your code.

[slade13]

Currently setUp() creates the whole strategy:

Map<String, Transaction> transactionMap = Map.of(
        "b", new BalanceTransaction(),
        "s", new SupplyTransaction(),
        "p", new PurchaseTransaction(),
        "r", new ReturnTransaction()
);

and then tests all four transactions through TransactionHandler.

The task specifically recommends keeping handler and strategy tests isolated and says that a map with one handler is enough.

Since BalanceTransaction, PurchaseTransaction, etc. already have their own test classes, TransactionHandlerTest doesn't need to verify their business logic again.

[slade13]

Avoid testing implementation-specific exceptions:

assertThrows(ArrayIndexOutOfBoundsException.class,
        () -> transactionHandler.process(line));

ArrayIndexOutOfBoundsException looks more like an accidental consequence of the current implementation than part of the application's contract.

A better implementation would probably validate the transaction format and throw something intentional such as:

IllegalArgumentException

Then the tests would verify business behavior rather than a side effect of array indexing.

This is partially an issue with production code, not only tests.

[slade13]

You improved this test class nicely, but WriterImplTest should ideally not use ReaderImpl.

WriterImplTest depends now on ReaderImpl, the task explicitly emphasizes isolated testing. Use Java directly:

List<String> reportLines = Files.readAllLines(Path.of(path));

After that WriterImplTest tests only WriterImpl and ReaderImpl already has its own test class.

[slade13]

Fix formatting issue:

Suggested change

	assertEquals(expectedReport,createReport(),
	assertEquals(expectedReport, createReport(),

[slade13]

Reformat this fragment of code:

Suggested change

	assertInstanceOf(BalanceTransaction.class,operationStrategy.getOperationType(input),
	assertInstanceOf(
	BalanceTransaction.class,
	operationStrategy.getOperationType(input),

[slade13]

Missing space:

Suggested change

	seedFruit(expectedFruit,startBalance);
	seedFruit(expectedFruit, startBalance);

[slade13]

Missing space:

Suggested change

	seedFruit(expectedFruit,startBalance);
	seedFruit(expectedFruit, startBalance);

[slade13]

Missing space:

Suggested change

	seedFruit(expectedFruit,startBalance);
	seedFruit(expectedFruit, startBalance);

[mateuszwojtkowiak]

Great job!