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.

Sign up for GitHub

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

Jump to bottom

Add NFT acceptance tests #2284

Merged
merged 7 commits into from
Jul 21, 2021
Merged

Add NFT acceptance tests #2284

merged 7 commits into from
Jul 21, 2021

Conversation

ijungmann
Copy link
Contributor

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

Add NFT acceptance tests

  • Add new acceptance tests for NFT feature

Related issue(s):

Fixes #2134

Checklist

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

Ian Jungmann added 2 commits July 16, 2021 10:31
Add NFT acceptance tests
d0fe491 
Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>
Add NFT acceptance tests
33e1fd1 
Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>
@codecov
Copy link

codecov bot commented Jul 16, 2021 •
edited
Loading

Codecov Report

Merging #2284 (e2f9768) into main (463be6a) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2284      +/-   ##
============================================
+ Coverage     83.48%   83.53%   +0.04%     
- Complexity     2234     2235       +1     
============================================
  Files           437      437              
  Lines         11998    11998              
  Branches       1002     1001       -1     
============================================
+ Hits          10017    10022       +5     
+ Misses         1653     1652       -1     
+ Partials        328      324       -4
Impacted Files Coverage Δ
...r/importer/config/MirrorImporterConfiguration.java 52.94% <0.00%> (-7.53%) ⬇️
...om/hedera/mirror/monitor/MirrorNodeProperties.java 47.36% <0.00%> (-5.58%) ⬇️
...om/hedera/mirror/importer/leader/LeaderAspect.java 0.00% <0.00%> (ø)
...rror/importer/config/HealthCheckConfiguration.java 100.00% <0.00%> (ø)
...ror/importer/config/StreamFileHealthIndicator.java 100.00% <0.00%> (ø)
...orter/config/CredentialsProviderConfiguration.java
...ror/importer/config/CloudStorageConfiguration.java 65.78% <0.00%> (ø)
.../hedera/mirror/importer/downloader/Downloader.java 86.10% <0.00%> (+0.10%) ⬆️
.../hedera/mirror/monitor/publish/PublishMetrics.java 86.51% <0.00%> (+1.73%) ⬆️
...mirror/monitor/subscribe/AbstractSubscription.java 82.85% <0.00%> (+1.90%) ⬆️
... and 1 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 5eae922...e2f9768. Read the comment docs.

ijungmann
ijungmann commented Jul 16, 2021
View reviewed changes
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated
.setTreasuryAccountId(treasuryAccount.getAccountId())
.setTransactionMemo(memo);

if (tokenType == TokenType.FUNGIBLE_COMMON) {
tokenCreateTransaction.setDecimals(10).setInitialSupply(initialSupply);
Copy link
Contributor Author

@ijungmann ijungmann Jul 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Multi-line

Copy link
Contributor

Choose a reason for hiding this comment

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

Should you also be setting TokenSupplyType to FINITE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily, both FUNGIBLE and NON_FUNGIBLE can be either INFINITE or FINITE. You bring up a good point that we should probably test both cases though, I just have it default everything to INFINITE for simplicity, but for completeness it would be good to ensure we can handle both. I'm about to push a test to add that check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added checks for that test.

ijungmann
ijungmann commented Jul 16, 2021
View reviewed changes
hedera-mirror-test/src/test/resources/features/hts/hts.feature
| 200 |

@acceptance @nft
Scenario Outline: Validate Update Treasury NFT Flow - Create, Associate, Mint, Update Treasury
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is temporarily a negative path test until services enables treasury updates for NFTs, not sure if we want to leave it so that we'll know to update it after or just remove it since we don't want to do a lot of negative path testing in here.

Copy link
Member

@steven-sheehy steven-sheehy Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We should not have negative tests as we're not testing HAPI. We're testing transactions that flow through to the mirror node.

@Nana-EC Nana-EC requested a review from a team July 16, 2021 16:18
@Nana-EC Nana-EC added enhancement Type: New feature nft Non-Fungible Token P2 test Test infrastructure, automated tests required, etc labels Jul 16, 2021
@Nana-EC Nana-EC added this to the Mirror 0.38.0 milestone Jul 16, 2021
Nana-EC
Nana-EC requested changes Jul 16, 2021
View reviewed changes
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Looking good on 1st pass, some suggestions.
I see you have some of your own comments to will swing back after those commits

hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated
.setTreasuryAccountId(treasuryAccount.getAccountId())
.setTransactionMemo(memo);

if (tokenType == TokenType.FUNGIBLE_COMMON) {
tokenCreateTransaction.setDecimals(10).setInitialSupply(initialSupply);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you also be setting TokenSupplyType to FINITE?

hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Outdated Show resolved Hide resolved
xin-hedera
xin-hedera requested changes Jul 16, 2021
View reviewed changes
...mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/MirrorNodeClient.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/steps/TokenFeature.java Outdated Show resolved Hide resolved
Ian Jungmann added 2 commits July 16, 2021 15:47
Feedback and cleanup
f71cba1 
Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>
Fix after feedback
c5df0c0 
Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>
Nana-EC
Nana-EC requested changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Looks good, some suggestions on TokenClient methods

hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated Show resolved Hide resolved
hedera-mirror-test/src/test/java/com/hedera/mirror/test/e2e/acceptance/client/TokenClient.java Outdated Show resolved Hide resolved
Ian Jungmann added 3 commits July 20, 2021 11:52
Swap overloading with explicit methods
a6671d5 
Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>
Cleanup
7df1747 
Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>
Break down token create
e2f9768 
Signed-off-by: Ian Jungmann <ian.jungmann@hedera.com>
@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2021

SonarCloud Quality Gate failed.

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 65 Code Smells

0.0% 0.0% Coverage
3.9% 3.9% Duplication

@ijungmann ijungmann requested a review from Nana-EC July 20, 2021 18:16
Nana-EC
Nana-EC approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LGTM

@ijungmann ijungmann merged commit 9d9f288 into main Jul 21, 2021
@ijungmann ijungmann deleted the nft_acceptance_tests_fixed branch July 21, 2021 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steven-sheehy steven-sheehy steven-sheehy approved these changes

@Nana-EC Nana-EC Nana-EC approved these changes

@xin-hedera xin-hedera xin-hedera approved these changes

Assignees

@ijungmann ijungmann

Labels
enhancement Type: New feature nft Non-Fungible Token P2 test Test infrastructure, automated tests required, etc
Projects
None yet
Milestone
Mirror 0.38.0
Development

Successfully merging this pull request may close these issues.

NFT acceptance tests
4 participants
@ijungmann @steven-sheehy @Nana-EC @xin-hedera