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

geth --dev test fixture setup #3191

Merged
merged 7 commits into from
Jan 30, 2024
Merged

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jan 17, 2024

What was wrong?

Related to Issue #3188

  • Could not build a geth fixture or run tests for latest geth versions using PoS where mining is not available
  • Prevented from running tests against latest geth due to some older commands / private chains needing a PoS client as well.

How was it fixed?

  • Running with geth --dev for local chain that uses simulated PoS
  • Fixture generation with geth running --dev mode
  • Tests with geth running --dev mode

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@fselmo fselmo force-pushed the messy-geth-dev-test-setup branch 6 times, most recently from 367bef4 to aa78a85 Compare January 18, 2024 00:07
@fselmo fselmo force-pushed the messy-geth-dev-test-setup branch 2 times, most recently from 6e906f8 to fd4eab0 Compare January 25, 2024 20:24
@fselmo
Copy link
Collaborator Author

fselmo commented Jan 25, 2024

^^ squashed and rebased

@reedsa reedsa force-pushed the messy-geth-dev-test-setup branch 3 times, most recently from cbba343 to abcce6d Compare January 25, 2024 22:53
@fselmo fselmo changed the title Messy geth dev test setup geth --dev test fixture setup Jan 25, 2024
@reedsa reedsa requested review from pacrob and kclowes January 25, 2024 23:54
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

lgtm! Take or leave geth version comments as you will.

fselmo and others added 5 commits January 26, 2024 13:15
…ing:

- Quicker timeout for unmined wait-for-txn-receipt tests
- Update default fees test for geth --dev mode
- Get rid of all miner start() and stop() references
- Run integration tests with --dev flag
- Test refactoring for ``geth --dev`` test fixture setup
- Remove PoW related endpoint tests; TODO: remove / deprecate endpoints
- Support latest geth versions
- Update geth version in circleCI config + peripherally related changes
- Turn on PoS block identifier tests
- get geth --dev test suite working
- Flaky for replace transaction
- Fix typing
- Loosen assertions on gas
- Nonce value fix
- Loosen asserts for default maxFeePerGas and maxPriorityFeePerGas tests
- Add @flaky to tests that expect mining at certain times
- This isn't ideal. Perhaps we can increase the ``dev.period`` (mining
  interval) to make these tests a bit more reliable and hopefully
  the other tests are unaffected.
- Update benchmark to run with ``geth --dev`` setup
- Put back old state of get_logs_without_logs for eth_tester
- Try with --dev.period=5
- Remove assert from test_eth_new_block_filter
- Remove sleep from test_web3_client_version
- Try without flaky
- Fixes test_eth_send_transaction_with_nonce without the need for retries.
- Retries were causing transactions to be sent with the same nonce,
  which in turn requires more gas to go through. Running the test once
  fixes this issue.
- Obtain the transaction count providing the 'pending' argument.
- Fix test_eth_send_transaction_no_max_fee
tests/integration/generate_fixtures/go_ethereum.py Outdated Show resolved Hide resolved
@@ -49,6 +52,14 @@
)


"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👌🏼

web3/tools/benchmark/node.py Show resolved Hide resolved
@fselmo
Copy link
Collaborator Author

fselmo commented Jan 26, 2024

lgtm but I can't review it since it's my branch 😆... green light from me though 👍🏼

@fselmo fselmo marked this pull request as ready for review January 26, 2024 21:17
reedsa added a commit to reedsa/web3.py that referenced this pull request Jan 26, 2024
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@reedsa reedsa merged commit 4410048 into ethereum:main Jan 30, 2024
81 checks passed
reedsa added a commit that referenced this pull request Jan 30, 2024
* WIP: Generate geth fixture for shanghai with clique consensus

* Initial squashed commits for getting ``geth --dev`` test fixture working:

- Quicker timeout for unmined wait-for-txn-receipt tests
- Update default fees test for geth --dev mode
- Get rid of all miner start() and stop() references
- Run integration tests with --dev flag
- Test refactoring for ``geth --dev`` test fixture setup
- Remove PoW related endpoint tests; TODO: remove / deprecate endpoints
- Support latest geth versions
- Update geth version in circleCI config + peripherally related changes
- Turn on PoS block identifier tests
- get geth --dev test suite working

* Updates towards ``geth --dev`` test fixture:

- Flaky for replace transaction
- Fix typing
- Loosen assertions on gas
- Nonce value fix
- Loosen asserts for default maxFeePerGas and maxPriorityFeePerGas tests

* ``geth --dev`` test fixture tweaks:

- Add @flaky to tests that expect mining at certain times
- This isn't ideal. Perhaps we can increase the ``dev.period`` (mining
  interval) to make these tests a bit more reliable and hopefully
  the other tests are unaffected.
- Update benchmark to run with ``geth --dev`` setup
- Put back old state of get_logs_without_logs for eth_tester

* More work towards ``geth --dev`` test fixture:

- Try with --dev.period=5
- Remove assert from test_eth_new_block_filter
- Remove sleep from test_web3_client_version
- Try without flaky
- Fixes test_eth_send_transaction_with_nonce without the need for retries.
- Retries were causing transactions to be sent with the same nonce,
  which in turn requires more gas to go through. Running the test once
  fixes this issue.
- Obtain the transaction count providing the 'pending' argument.
- Fix test_eth_send_transaction_no_max_fee

* Additional cleanup for ``geth --dev`` test fixture

* Sync with feedback in #3191 for ``geth --dev`` test fixture

---------

Co-authored-by: fselmo <[email protected]>
@fselmo fselmo deleted the messy-geth-dev-test-setup branch April 3, 2024 20:49
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.

4 participants