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

Ensure acceptance tests run on previewnet/testnet #351

Merged
merged 7 commits into from
Jul 20, 2022

Conversation

Ivo-Yankov
Copy link
Collaborator

@Ivo-Yankov Ivo-Yankov commented Jul 19, 2022

Signed-off-by: Ivo Yankov [email protected]

Description:
Adjust the acceptance tests so that they run consistently against Testnet and Previewnet.
Some tests were failing with error -32009: Gas price too low due to a higher gas price on the public networks than the default hardcoded one that is used when testing against the local node.

When testing against Previewnet it turned out that the chainId value in the README was incorrect.

Related issue(s):

Fixes #340

Notes for reviewer:

Checklist

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

@Ivo-Yankov Ivo-Yankov added enhancement New feature or request P1 process Build, test and deployment-process related tasks labels Jul 19, 2022
@Ivo-Yankov Ivo-Yankov added this to the 0.5.0 milestone Jul 19, 2022
@Ivo-Yankov Ivo-Yankov added this to In progress in Development via automation Jul 19, 2022
@Ivo-Yankov Ivo-Yankov self-assigned this Jul 19, 2022
@Ivo-Yankov Ivo-Yankov marked this pull request as ready for review July 19, 2022 15:04
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2022

Codecov Report

Merging #351 (92c7735) into main (cc9ba38) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #351   +/-   ##
=======================================
  Coverage   62.35%   62.35%           
=======================================
  Files           9        9           
  Lines         874      874           
  Branches      143      143           
=======================================
  Hits          545      545           
  Misses        291      291           
  Partials       38       38           

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 cc9ba38...92c7735. Read the comment docs.

shemnon
shemnon previously approved these changes Jul 19, 2022
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.
Questions and a suggestion

packages/server/tests/acceptance/erc20.spec.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/rpc.spec.ts Outdated Show resolved Hide resolved
@@ -406,8 +407,10 @@ describe('RPC Server Acceptance Tests', function () {
const transaction = {
...default155TransactionData,
to: mirrorContract.evm_address,
nonce: await relay.getAccountNonce(accounts[2].address)
nonce: await relay.getAccountNonce(accounts[2].address),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was lack of gasPrice working before.
Please add to comments section of PR something to highlight this issue and solution


const transaction = {
...defaultLondonTransactionData,
to: mirrorContract.evm_address,
nonce: await relay.getAccountNonce(accounts[2].address)
nonce: await relay.getAccountNonce(accounts[2].address),
maxPriorityFeePerGas: gasPrice,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar PR comments for this section as to why it works in local node but is an issue here

packages/server/tests/acceptance/rpc.spec.ts Outdated Show resolved Hide resolved
# Conflicts:
#	packages/server/tests/helpers/assertions.ts
@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Ivo-Yankov Ivo-Yankov requested a review from Nana-EC July 20, 2022 14:24
@Ivo-Yankov Ivo-Yankov moved this from In progress to For Review in Development Jul 20, 2022
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.

LG
Please add in the PR comments also why adding the gasPrice, maxPriorityFeePerGas and maxFeePerGas now fixes it yet they were passing locally before

@Ivo-Yankov
Copy link
Collaborator Author

Since the gas price is not static for the public networks some of the tests for eth_sendRawTransaction were failing with -32009: Gas price too low. The solution was the fields gasPrice, maxPriorityFeePerGas and maxFeePerGas to have their respective default values overwritten with an up-to-date gas price from eth_gasPrice.

@Ivo-Yankov Ivo-Yankov merged commit d467b65 into main Jul 20, 2022
Development automation moved this from For Review to Done Jul 20, 2022
@Ivo-Yankov Ivo-Yankov deleted the 340-acceptance-tests-against-public-nets branch July 20, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 process Build, test and deployment-process related tasks
Projects
Development

Successfully merging this pull request may close these issues.

Verify consistent passes of acceptance tests against previewnet/testnet
4 participants