Remove data generator #2387

ijungmann · 2021-08-10T16:33:34Z

Description:
Remove data generator module and add test cases for TransactionSuppliers

Remove the data generator module and all references to it.
Move TransactionSuppliers and anything else needed by the monitor into the monitor module
Add unit tests for the TransactionSuppliers

Related issue(s):

Fixes #2312

Checklist

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

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

.../hedera/mirror/monitor/publish/transaction/account/AccountUpdateTransactionSupplierTest.java

...hedera/mirror/monitor/publish/transaction/account/CryptoTransferTransactionSupplierTest.java

ijungmann · 2021-08-10T16:38:08Z

.../hedera/mirror/monitor/publish/transaction/account/AccountCreateTransactionSupplierTest.java

+class AccountCreateTransactionSupplierTest extends AbstractTransactionSupplierTest {
+
+    @Test
+    void createWithMinimumData() {


Should probably be Minimal, no Minimum

codecov · 2021-08-10T16:40:58Z

Codecov Report

Merging #2387 (b5c7073) into main (922abc5) will increase coverage by 6.27%.
The diff coverage is 97.22%.

❗ Current head b5c7073 differs from pull request most recent head 91c311e. Consider uploading reports for the commit 91c311e to get more accurate results

@@             Coverage Diff              @@
##               main    #2387      +/-   ##
============================================
+ Coverage     84.42%   90.69%   +6.27%     
- Complexity     2335     2424      +89     
============================================
  Files           440      419      -21     
  Lines         12018    11578     -440     
  Branches       1024     1009      -15     
============================================
+ Hits          10146    10501     +355     
+ Misses         1554      745     -809     
- Partials        318      332      +14

Impacted Files	Coverage Δ
...or/monitor/expression/ExpressionConverterImpl.java	`94.80% <ø> (ø)`
...r/properties/ScenarioPropertiesAggregatorImpl.java	`100.00% <ø> (ø)`
.../hedera/mirror/monitor/publish/PublishMetrics.java	`85.36% <ø> (ø)`
...ror/monitor/publish/PublishScenarioProperties.java	`94.11% <ø> (ø)`
...sh/generator/ConfigurableTransactionGenerator.java	`100.00% <ø> (ø)`
...r/monitor/publish/transaction/TransactionType.java	`100.00% <ø> (ø)`
...tion/account/AccountCreateTransactionSupplier.java	`68.18% <ø> (ø)`
...tion/account/AccountDeleteTransactionSupplier.java	`63.63% <ø> (ø)`
...tion/account/AccountUpdateTransactionSupplier.java	`77.27% <ø> (ø)`
...ensus/ConsensusCreateTopicTransactionSupplier.java	`78.94% <ø> (ø)`
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 922abc5...91c311e. Read the comment docs.

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

ijungmann · 2021-08-10T16:59:34Z

Will address code smells

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

steven-sheehy · 2021-08-10T17:40:16Z

Merging #2387 (88d4072) into main (10cbbb0) will increase coverage by 7.44%.

92% coverage now. Nice!

ijungmann · 2021-08-10T17:40:48Z

Merging #2387 (88d4072) into main (10cbbb0) will increase coverage by 7.44%.

92% coverage now. Nice!

Yeah! Much nicer number.

Nana-EC

Seems there were merge issue with copyrights details.

Nana-EC · 2021-08-10T19:44:20Z

hedera-mirror-monitor/src/main/java/com/hedera/mirror/monitor/util/Utility.java

 * ‌
 * Hedera Mirror Node
 * 
- * Copyright (C) 2019 - 2021 Hedera Hashgraph, LLC
+ * Copyright (C) 2019 - 2020 Hedera Hashgraph, LLC


Fixed. I've removed the setup I had to add this automatically as it had all sorts of issues so this doesn't crop up again.

Nana-EC · 2021-08-10T19:44:45Z

...test/java/com/hedera/mirror/monitor/publish/transaction/AbstractTransactionSupplierTest.java

+ * ‌
+ * Hedera Mirror Node
+ * 
+ * Copyright (C) 2019 - 2020 Hedera Hashgraph, LLC


Copy right repeated. Should also be 2021

...test/java/com/hedera/mirror/monitor/publish/transaction/AbstractTransactionSupplierTest.java

hedera-mirror-monitor/src/test/java/com/hedera/mirror/monitor/util/UtilityTest.java

steven-sheehy · 2021-08-10T21:43:12Z

.../hedera/mirror/monitor/publish/transaction/account/AccountCreateTransactionSupplierTest.java

+        AccountCreateTransaction expected = new AccountCreateTransaction()
+                .setAccountMemo(actual.getAccountMemo())
+                .setInitialBalance(ONE_TINYBAR)
+                .setKey(key)
+                .setMaxTransactionFee(ONE_TINYBAR)
+                .setReceiverSignatureRequired(true)
+                .setTransactionMemo(actual.getTransactionMemo());
+
+        assertAll(
+                () -> assertThat(actual.getTransactionMemo()).contains("Mirror node created test account"),
+                () -> assertThat(actual.getAccountMemo()).contains("Mirror node created test account"),
+                () -> assertThat(actual).usingRecursiveComparison().isEqualTo(expected)
+        );


I don't like creating a temporary object just do do equality on it. I'm also not a fan of assertAll. It would be simpler to do a single chaining assert to verify results:

assertThat(actual) .returns(key, AccountCreateTransaction::getKey) .returns(ONE_TINYBAR, AccountCreateTransaction::getMaxTransactionFee) ...

You can chain non-equality assertions like contains using satisfies(a -> assertThat(a.get...()) or using extracting() at the end.

The thing I like about creating the temporary object and doing equality on it is it forces someone to set every field, it protects against future fields or some of the fields that aren't as explicit (like setting adminKey in a supplier also gets used for submitKey, freezeKey, etc.). The other thing I'm running into as I've tried to convert over, TokenNftTransfer is not a public class so I don't know of a great way to go field-by-field for that.

I do see the value in the chaining you mentioned, that can be done.

That logic makes sense in general, but these SDK objects are quite complex and I'm not sure we want to rely on their internals being stable. They don't implement equals()/hashCode() to provide proper equality and they contain a lot of internal, non-settable fields like proto builders, node account IDs, signed transactions, signatures, etc. that should not be compared.

I don't think we're trying to validate quite to the level of every field. My intent with the ticket was just to test that the few fields we set are getting passed through. Validating everything might become brittle as the SDK changes things internally.

I've opened hashgraph/hedera-sdk-java#603 to address the TokenNftTransfer issue. If you see such minor issues with the SDK you can always just submit a PR to fix it.

That's fair, I did have a to do something different with ScheduleCreate for that reason, too many internal fields that I could not get a hold of to even set the expected and all of the extra fields you mentioned. I'll work on swapping things over to just checking fields and will keep an eye out for other bugs.

2.0.11 of sdk was released with my fix for public class.

Reworked the tests to use the chaining suggested.

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

...irror-monitor/src/main/java/com/hedera/mirror/monitor/publish/PublishScenarioProperties.java

.../hedera/mirror/monitor/publish/transaction/account/AccountCreateTransactionSupplierTest.java

steven-sheehy · 2021-08-11T04:13:54Z

.../hedera/mirror/monitor/publish/transaction/account/AccountCreateTransactionSupplierTest.java

+                .satisfies(a -> assertThat(a.getAccountMemo()).contains("Mirror node created test account"))
+                .satisfies(a -> assertThat(a.getKey()).isNotNull())
+                .satisfies(a -> assertThat(a.getTransactionMemo()).contains("Mirror node created test account"))
+                .satisfies(a -> assertThat(a).usingRecursiveComparison().isEqualTo(expected));


Suggested change

.satisfies(a -> assertThat(a).usingRecursiveComparison().isEqualTo(expected));

.usingRecursiveComparison()

.isEqualTo(expected));

Removed this line.

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

.../hedera/mirror/monitor/publish/transaction/account/AccountDeleteTransactionSupplierTest.java

.../hedera/mirror/monitor/publish/transaction/account/AccountUpdateTransactionSupplierTest.java

...hedera/mirror/monitor/publish/transaction/account/CryptoTransferTransactionSupplierTest.java

...ror/monitor/publish/transaction/consensus/ConsensusSubmitMessageTransactionSupplierTest.java

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

sonarcloud · 2021-08-18T17:28:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.8% Duplication

ijungmann · 2021-08-18T17:31:40Z

...va/com/hedera/mirror/monitor/publish/transaction/token/TokenMintTransactionSupplierTest.java

+                .extracting(TokenMintTransaction::getMetadata)
+                .returns(2, List::size)
+                .returns(14, metadataList -> metadataList.get(0).length)
+                .returns(14, metadataList -> metadataList.get(1).length);


Haven't found a good chain to check this yet, if I cast it to a list in the extracting it won't let me use returns, which then I didn't see a great way to extract the length of each the byte array cleanly. Suggestions are welcome, this is the only test that hasn't been refactored.

ijungmann · 2021-08-18T17:35:00Z

...java/com/hedera/mirror/monitor/publish/transaction/token/TokenUpdateTransactionSupplier.java

-                .setAutoRenewPeriod(autoRenewPeriod)
-                .setExpirationTime(expirationTime)


expirationTime is coerced by the autoRenewPeriod, if setExpirationTime is called it also sets autoRenewPeriod to null, so setting both does nothing. I'll have a separate PR to address this in a couple other spots I saw in other modules.

steven-sheehy

LGTM

xin-hedera

LGTM

Nana-EC

LGTM

* Remove data generator (#2387) * Remove the data generator module and all references to it. * Move TransactionSuppliers and anything else needed by the monitor into the monitor module * Add unit tests for the TransactionSuppliers Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com> * Fix startDate logic on empty DB (#2421) * Fix startDate logic on empty DB The current MirrorDateRangePropertiesProcessor.newDateRangeFilter() currently doesn't honor the startDate set on an empty db - Update newDateRangeFilter() logic to handle case when there's no lastFileInstant - Add a test case to verify scenario Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com> * Remove transactionMemo from TransactionSupplier and expirationTime from TokenClient (#2423) * Remove setting transactionMemo in TransactionSuppliers * Remove setting expirationTime in TokenClient Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com> * Fix rest service startup crash (#2425) - replace the one-time verifyDbConnection with on-demand transaction result & type loading Signed-off-by: Xin Li <xin.li@hedera.com> * Add missing token account association migration (#2424) - add a db migration script to add the missing token account associations Signed-off-by: Xin Li <xin.li@hedera.com> Co-authored-by: Ian Jungmann <ian.jungmann@hedera.com> Co-authored-by: Nana-EC <56320167+Nana-EC@users.noreply.github.com> Co-authored-by: Xin Li <59580070+xin-hedera@users.noreply.github.com>

Ian Jungmann added 5 commits August 9, 2021 09:40

Remover datagenerator and add first test

70746cc

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Add a bunch of tests and tweak attributes

a6682f7

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Add schedule tests and fixeds

592c222

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Remove datagenerator

da5d7de

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Merge branch 'main' into remove_data_generator

bf3bfb5

ijungmann self-assigned this Aug 10, 2021

ijungmann added monitor Area: Monitoring and dashboard P3 labels Aug 10, 2021

ijungmann added this to the Mirror 0.39.0 milestone Aug 10, 2021

ijungmann commented Aug 10, 2021

View reviewed changes

.../hedera/mirror/monitor/publish/transaction/account/AccountUpdateTransactionSupplierTest.java Outdated Show resolved Hide resolved

ijungmann commented Aug 10, 2021

View reviewed changes

...hedera/mirror/monitor/publish/transaction/account/CryptoTransferTransactionSupplierTest.java Outdated Show resolved Hide resolved

ijungmann commented Aug 10, 2021

View reviewed changes

Test fixes

a988170

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

ijungmann requested a review from a team August 10, 2021 16:53

Add back Utility tests

a0e3f8e

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Move Utility into a package and fix tests

4d7fcf4

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

steven-sheehy added the enhancement Type: New feature label Aug 10, 2021

Nana-EC requested changes Aug 10, 2021

View reviewed changes

steven-sheehy requested changes Aug 10, 2021

View reviewed changes

Ian Jungmann added 5 commits August 10, 2021 18:25

Fix headers

d00ba1e

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Fix headers

f3745ca

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Remove newlines between static final fields

82c0eeb

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Switch account tests to chaining

f0a9913

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Chain the rest of the tests

ae121a0

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

xin-hedera requested changes Aug 11, 2021

View reviewed changes

...irror-monitor/src/main/java/com/hedera/mirror/monitor/publish/PublishScenarioProperties.java Outdated Show resolved Hide resolved

.../hedera/mirror/monitor/publish/transaction/account/AccountCreateTransactionSupplierTest.java Outdated Show resolved Hide resolved

steven-sheehy requested changes Aug 11, 2021

View reviewed changes

Ian Jungmann added 5 commits August 17, 2021 16:28

Do field-by-field checks

962a04b

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Merge branch 'main' into remove_data_generator

892ea7c

Add null checks

a588955

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Reorder imports

fb27723

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Code smells

cc06f59

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

ijungmann requested review from steven-sheehy, xin-hedera and Nana-EC August 18, 2021 03:57

Reorder asserts

f40f75b

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

steven-sheehy requested changes Aug 18, 2021

View reviewed changes

Ian Jungmann added 2 commits August 18, 2021 10:38

Remove transaction memo checks

c0675d6

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

Clean up tests

91c311e

Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>

ijungmann commented Aug 18, 2021

View reviewed changes

ijungmann requested a review from steven-sheehy August 18, 2021 17:35

steven-sheehy approved these changes Aug 18, 2021

View reviewed changes

xin-hedera approved these changes Aug 18, 2021

View reviewed changes

Nana-EC approved these changes Aug 18, 2021

View reviewed changes

ijungmann merged commit dc8570c into main Aug 18, 2021

ijungmann deleted the remove_data_generator branch August 18, 2021 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove data generator #2387

Remove data generator #2387

ijungmann commented Aug 10, 2021 •

edited

Loading

ijungmann Aug 10, 2021

codecov bot commented Aug 10, 2021 •

edited

Loading

ijungmann commented Aug 10, 2021

steven-sheehy commented Aug 10, 2021

ijungmann commented Aug 10, 2021

Nana-EC left a comment

Nana-EC Aug 10, 2021

ijungmann Aug 10, 2021

Nana-EC Aug 10, 2021

ijungmann Aug 10, 2021

steven-sheehy Aug 10, 2021

ijungmann Aug 11, 2021

steven-sheehy Aug 11, 2021 •

edited

Loading

ijungmann Aug 11, 2021

steven-sheehy Aug 12, 2021

ijungmann Aug 17, 2021

steven-sheehy Aug 11, 2021

ijungmann Aug 17, 2021

sonarcloud bot commented Aug 18, 2021

ijungmann Aug 18, 2021

ijungmann Aug 18, 2021

steven-sheehy left a comment

xin-hedera left a comment

Nana-EC left a comment

	.satisfies(a -> assertThat(a).usingRecursiveComparison().isEqualTo(expected));
	.usingRecursiveComparison()
	.isEqualTo(expected));

		.setAutoRenewPeriod(autoRenewPeriod)
		.setExpirationTime(expirationTime)

Remove data generator #2387

Remove data generator #2387

Conversation

ijungmann commented Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2021 • edited Loading

Codecov Report

ijungmann commented Aug 10, 2021

steven-sheehy commented Aug 10, 2021

ijungmann commented Aug 10, 2021

Nana-EC 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

steven-sheehy Aug 11, 2021 • edited Loading

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

sonarcloud bot commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-sheehy left a comment

Choose a reason for hiding this comment

xin-hedera left a comment

Choose a reason for hiding this comment

Nana-EC left a comment

Choose a reason for hiding this comment

ijungmann commented Aug 10, 2021 •

edited

Loading

codecov bot commented Aug 10, 2021 •

edited

Loading

steven-sheehy Aug 11, 2021 •

edited

Loading