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.

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

test: Extend the coverage of account nonce discrepancies tests #703

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

nickeynikolovv
Copy link

This PR extends the coverage of account nonce discrepancies tests according to test plan - https://www.notion.so/limechain/Address-account-nonce-discrepancies-Test-Plan-4a186e8cfabc49888ba501750f68d8de?pvs=4

Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

Nice work but some comments

contracts/discrepancies/nonce/InternalCallee.sol Outdated Show resolved Hide resolved
contracts/discrepancies/nonce/InternalCallee.sol Outdated Show resolved Hide resolved
contracts/discrepancies/nonce/InternalCallee.sol Outdated Show resolved Hide resolved
contracts/discrepancies/nonce/InternalCallee.sol Outdated Show resolved Hide resolved
anastasiya-kovaliova and others added 6 commits March 25, 2024 16:08
Signed-off-by: Anastasia Kovaliova <[email protected]>
Signed-off-by: Anastasia Kovaliova <[email protected]>
- added and adapted internalTransfer function in internalCallee contract
- skipped the blocked tests due to some issues

Signed-off-by: Nikolay Nikolov <[email protected]>
- refactored some expected to fail function in test 030

Signed-off-by: Nikolay Nikolov <[email protected]>
Signed-off-by: Nikolay Nikolov <[email protected]>
@nickeynikolovv nickeynikolovv force-pushed the test_677-account-nonce-discrepancies-additional-tests branch from fa0b04c to 6c4a873 Compare March 25, 2024 14:17
Copy link

github-actions bot commented Mar 25, 2024

Test Results

  14 files  ±  0    74 suites  +2   7m 46s ⏱️ -28s
244 tests +  2  238 ✔️ +  2  6 💤 ±0  0 ±0 
310 runs  +68  304 ✔️ +68  6 💤 ±0  0 ±0 

Results for commit 7c49202. ± Comparison against base commit 21aaeb3.

♻️ This comment has been updated with latest results.

quiet-node
quiet-node previously approved these changes Mar 26, 2024
Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LG! Great work thanks!

Comment on lines 386 to 390
erc721Contract = await Utils.deployERC721Contract();
tokenAddress = await Utils.createNonFungibleToken(
tokenCreateContract,
signers[0].address
);
Copy link
Contributor

@victor-yanev victor-yanev Apr 4, 2024

Choose a reason for hiding this comment

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

Let's avoid overriding variables which are reused in multiple tests - this creates dependencies between the tests where, depending on their execution order, the variable might hold a different value. This reduces the readability of code and makes it hard to trace what is happening without knowing the order of the tests or without going into debug mode.

If we are going to reuse a variable let's assign it only in one place (e.g., in a before() method or, where possible, directly turn it into a const.)

If there are subsets of tests which expect different values here for these variables, we can group them in multiple describes and declare + assign those variables there.

Please check for other such occurrences and try to fix them 🙂. Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

Unneeded override is removed

Choose a reason for hiding this comment

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

right


//NONCE-023
//need to update the get contract nonce - waiting for SDK
xit('should update contract nonce for each deployed contract', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a lil reminder to enable this later (and other tests waiting for the SDK changes)

Copy link
Collaborator

@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, a few comments to address.

contracts/discrepancies/nonce/ChainedContracts.sol Outdated Show resolved Hide resolved
test/hts-precompile/utils.js Show resolved Hide resolved
test/discrepancies/Nonce.js Show resolved Hide resolved
-added comments for additional info
-removed test 29

Signed-off-by: Nikolay Nikolov <[email protected]>
@Nana-EC
Copy link
Collaborator

Nana-EC commented Jun 20, 2024

@nickeynikolovv please rebase

@quiet-node
Copy link
Member

Hello @nickeynikolovv is this PR still relavant? If so please kindly update the PR. Thanks!

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.

6 participants