Skip to content

Conversation

@Ariho-Seth
Copy link
Contributor

@Ariho-Seth Ariho-Seth commented May 26, 2025

A new test class SmartContractClientImplTest was created for SmartContractClientImpl and tests for the three overloaded createContract methods. The coverage for all input types (i.e., FileId, byte array, and Path) has been tested and both positive and negative scenarios.

For tests that require a path parameter, test file is created first i.e. tempPath= Files.createTempFile("testContract", ".bin"); which is used during the tests and then is later on deleted after the test
View issue here

@Ariho-Seth
Copy link
Contributor Author

Hello @Ndacyayisenga-droid or @hendrikebbers , may you please review the changes added here and tell me incase anything needs to be added or removed

Copy link
Member

@hendrikebbers hendrikebbers left a comment

Choose a reason for hiding this comment

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

IFileReader creates unneeded complexity and is only used for mocks. We should try to avoid it.

import java.io.IOException;
import java.nio.file.Path;

public interface IFileReader {
Copy link
Member

Choose a reason for hiding this comment

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

I disagree with this approach. Instead of mocking the Fiel read in the test you can easily create a test file and use that one in the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, and I've worked on them as requested.

@Ariho-Seth
Copy link
Contributor Author

Ariho-Seth commented May 31, 2025

Thanks @hendrikebbers for the suggestions, Please check thru the updated code and feel free to leave a review incase of any changes needed.

import com.openelements.hiero.base.NftClient;
import com.openelements.hiero.base.SmartContractClient;
import com.openelements.hiero.base.TopicClient;
import com.openelements.hiero.base.*;
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use * imports

import com.openelements.hiero.base.HieroContext;
import com.openelements.hiero.base.NftClient;
import com.openelements.hiero.base.SmartContractClient;
import com.openelements.hiero.base.*;
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use * imports

@Ariho-Seth
Copy link
Contributor Author

Ohh yeah!, that's true
I've fixed the imports, How about now?

Signed-off-by: Atwijukire Ariho Seth <[email protected]>
Signed-off-by: Atwijukire Ariho Seth <[email protected]>
@Ariho-Seth Ariho-Seth requested a review from hendrikebbers June 26, 2025 14:24
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.

2 participants