refactor: enhance migration testing tool #17246

mustafauzunn · 2025-01-07T12:11:37Z

Description:

Enhance the migration testing tool to work with new transactions of bytes.

Related issue(s):

Fixes #16842

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: Mustafa Uzun <[email protected]>

codecov · 2025-01-07T12:22:05Z

Codecov Report

Attention: Patch coverage is 76.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 67.43%. Comparing base (be1981d) to head (f2dc3db).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rlds/demo/migration/MigrationTestingToolState.java	72.22%	3 Missing and 2 partials ⚠️
...irlds/demo/migration/MigrationTestingToolMain.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #17246      +/-   ##
============================================
+ Coverage     67.41%   67.43%   +0.02%     
- Complexity    22063    22076      +13     
============================================
  Files          2585     2585              
  Lines         96408    96433      +25     
  Branches      10071    10073       +2     
============================================
+ Hits          64992    65031      +39     
+ Misses        27698    27682      -16     
- Partials       3718     3720       +2

Files with missing lines	Coverage Δ
...a/com/swirlds/demo/migration/TransactionUtils.java	`72.72% <100.00%> (+32.72%)`	⬆️
...irlds/demo/migration/MigrationTestingToolMain.java	`0.00% <0.00%> (ø)`
...rlds/demo/migration/MigrationTestingToolState.java	`32.98% <72.22%> (+32.98%)`	⬆️

... and 1 file with indirect coverage changes

codacy-production · 2025-01-07T12:22:17Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.02% (target: -1.00%)	✅ 84.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`be1981d`)	96191	68558	71.27%
Head commit (`f2dc3db`)	96216 (+25)	68599 (+41)	71.30% (+0.02%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#17246)	25	21	84.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

…2-enhance-migration-testing-tool

Signed-off-by: Mustafa Uzun <[email protected]>

…2-enhance-migration-testing-tool

Signed-off-by: Mustafa Uzun <[email protected]>

andrewb1269hg

Review and approve file platform-sdk/platform-apps/tests/MigrationTestingTool/build.gradle.kts

…2-enhance-migration-testing-tool # Conflicts: # platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/system/SwirldMain.java

Signed-off-by: Mustafa Uzun <[email protected]>

IvanKavaldzhiev · 2025-01-10T14:29:47Z

.../MigrationTestingTool/src/main/java/com/swirlds/demo/migration/MigrationTestingToolMain.java

@@ -207,4 +209,9 @@ public PlatformMerkleStateRoot newMerkleStateRoot() {
    public BasicSoftwareVersion getSoftwareVersion() {
        return softwareVersion;
    }
+
+    @Override
+    public Bytes encodeSystemTransaction(@NonNull StateSignatureTransaction transaction) {


Add final for the argument and nonnull annotation for the return type

IvanKavaldzhiev · 2025-01-10T14:30:01Z

...MigrationTestingTool/src/main/java/com/swirlds/demo/migration/MigrationTestingToolState.java

+    @Override
+    public void preHandle(
+            @NonNull Event event,
+            @NonNull Consumer<ScopedSystemTransaction<StateSignatureTransaction>> stateSignatureTransaction) {


Make all arguments final

IvanKavaldzhiev · 2025-01-10T14:30:55Z

...MigrationTestingTool/src/main/java/com/swirlds/demo/migration/MigrationTestingToolState.java

+            @NonNull Event event,
+            @NonNull Consumer<ScopedSystemTransaction<StateSignatureTransaction>> stateSignatureTransaction) {
+        event.forEachTransaction(transaction -> {
+            if (!transaction.isSystem() && isSystemTransaction(transaction.getApplicationTransaction())) {


You can extract the isSystem() check as a separate code before the if statement and add a comment on top of it - about why we check whether it's the deprecated system transaction format

IvanKavaldzhiev · 2025-01-10T14:31:39Z

...MigrationTestingTool/src/main/java/com/swirlds/demo/migration/MigrationTestingToolState.java

@@ -335,4 +355,18 @@ public int getVersion() {
    public int getMinimumSupportedVersion() {
        return ClassVersion.VIRTUAL_MAP;
    }
+
+    private void consumeSystemTransaction(
+            final Transaction transaction,


Add nonnull annotations for all arguments

IvanKavaldzhiev · 2025-01-10T14:44:26Z

...ps/tests/MigrationTestingTool/src/main/java/com/swirlds/demo/migration/TransactionUtils.java

+    public static boolean isSystemTransaction(@NonNull final Bytes bytes) {
+        final SerializableDataInputStream in = new SerializableDataInputStream(bytes.toInputStream());
+
+        try {


It might be performance heavy to try to serialize every transaction that is incoming with having a try/catch block. You can add a marker byte as @lpetrovic05 suggested in another PR.

IvanKavaldzhiev · 2025-01-10T14:44:48Z

...ationTestingTool/src/test/java/com/swirlds/demo/migration/MigrationTestingToolStateTest.java

+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+public class MigrationTestingToolStateTest {


public can be removed

IvanKavaldzhiev · 2025-01-10T14:45:28Z

...ationTestingTool/src/test/java/com/swirlds/demo/migration/MigrationTestingToolStateTest.java

+import org.mockito.Mockito;
+
+public class MigrationTestingToolStateTest {
+    private static MigrationTestingToolState state;


Why do we need the state to be static?

Signed-off-by: Mustafa Uzun <[email protected]>

refactor: enhance migration testing tool

6c47032

Signed-off-by: Mustafa Uzun <[email protected]>

mustafauzunn added the Platform Tickets pertaining to the platform label Jan 7, 2025

mustafauzunn added this to the v0.59 milestone Jan 7, 2025

mustafauzunn self-assigned this Jan 7, 2025

mustafauzunn added 6 commits January 7, 2025 14:22

Merge branch 'main' of github.com:hashgraph/hedera-services into 1684…

81ba16d

…2-enhance-migration-testing-tool

test: add unit tests for the new logic in handle and prehandle

77ce485

Signed-off-by: Mustafa Uzun <[email protected]>

refactor: dependency

2a91419

Signed-off-by: Mustafa Uzun <[email protected]>

Merge branch 'main' of github.com:hashgraph/hedera-services into 1684…

5bd552e

…2-enhance-migration-testing-tool

test: refactor unit tests for the new logic in handle and prehandle

6caef12

Signed-off-by: Mustafa Uzun <[email protected]>

test: add unit tests for application transaction

baab5b2

Signed-off-by: Mustafa Uzun <[email protected]>

mustafauzunn marked this pull request as ready for review January 9, 2025 12:28

mustafauzunn requested review from a team as code owners January 9, 2025 12:28

mustafauzunn requested review from rbarkerSL and lpetrovic05 January 9, 2025 12:28

test: refactor unit tests

41a373c

Signed-off-by: Mustafa Uzun <[email protected]>

andrewb1269hg previously approved these changes Jan 9, 2025

View reviewed changes

mustafauzunn added 2 commits January 10, 2025 13:34

Merge branch 'main' of github.com:hashgraph/hedera-services into 1684…

f431ee8

…2-enhance-migration-testing-tool # Conflicts: # platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/system/SwirldMain.java

refactor: implementation

41599d5

Signed-off-by: Mustafa Uzun <[email protected]>

mustafauzunn dismissed andrewb1269hg’s stale review via 41599d5 January 10, 2025 11:38

IvanKavaldzhiev reviewed Jan 10, 2025

View reviewed changes

refactor: PR comments

f2dc3db

Signed-off-by: Mustafa Uzun <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: enhance migration testing tool #17246

refactor: enhance migration testing tool #17246

mustafauzunn commented Jan 7, 2025

codecov bot commented Jan 7, 2025 •

edited

Loading

codacy-production bot commented Jan 7, 2025 •

edited

Loading

andrewb1269hg left a comment

IvanKavaldzhiev Jan 10, 2025

mustafauzunn Jan 10, 2025

IvanKavaldzhiev Jan 10, 2025

mustafauzunn Jan 10, 2025

IvanKavaldzhiev Jan 10, 2025

mustafauzunn Jan 10, 2025

IvanKavaldzhiev Jan 10, 2025

mustafauzunn Jan 10, 2025

IvanKavaldzhiev Jan 10, 2025

IvanKavaldzhiev Jan 10, 2025

IvanKavaldzhiev Jan 10, 2025

refactor: enhance migration testing tool #17246

Are you sure you want to change the base?

refactor: enhance migration testing tool #17246

Conversation

mustafauzunn commented Jan 7, 2025

codecov bot commented Jan 7, 2025 • edited Loading

Codecov Report

codacy-production bot commented Jan 7, 2025 • edited Loading

Coverage summary from Codacy

andrewb1269hg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 7, 2025 •

edited

Loading

codacy-production bot commented Jan 7, 2025 •

edited

Loading