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

Fix tests from 1835 changes and default to 1559 fields for testing #2053

Merged

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jun 25, 2021

What was wrong?

Related to Issue #1835

  1. We skipped some tests in order to create the working london branch. Needed to isolate what was going wrong.
  2. We should update default params in our existing transactions in our tests to use the new fields

How was it fixed?

  1. Some older tests seemed to be breaking others because they did not meet updated requirements to be included in a pending block, seemingly creating a txn jam. Increasing gasPrice for these tests seemed to move things along. Another issue we have is that some tests have their context dirtied by other tests. I included a way to move some tests to the beginning of the test suite to prevent this dirtied context and allow these tests to pass.

  2. Updated default tests to use the new 1559 fields for our transactions.

Todo:

  • Fix broken / skipped tests from london branch creation
  • Update existing tests to use the new 1559 fields maxFeePerGas and maxPriorityFeePerGas over gasPrice as defaults
  • Added documentation on unit and integration testing and how to contribute to our test suite
  • Add entry to the release notes

Cute Animal Picture

IMG_20210615_092412_01

@fselmo fselmo force-pushed the 1835-fix-tests-and-default-to-1559-fields branch 9 times, most recently from 5af1986 to 44a93ac Compare June 29, 2021 20:33
@fselmo fselmo changed the title [WIP] 1835 fix tests and default to 1559 fields Fix tests from 1835 changes and default to 1559 fields for testing Jun 29, 2021
@fselmo fselmo force-pushed the 1835-fix-tests-and-default-to-1559-fields branch 2 times, most recently from 1277e66 to f1b4040 Compare June 29, 2021 22:05
@fselmo fselmo requested a review from kclowes June 29, 2021 22:15
@fselmo fselmo force-pushed the 1835-fix-tests-and-default-to-1559-fields branch 2 times, most recently from 9e4149a to ca957fb Compare June 30, 2021 00:27
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.

Awesome! Thanks for being so persistent with these tests! I just left one comment about putting back the error expectation that you can take or leave, whatever makes sense

try:
web3.geth.miner.start()
super().test_eth_replace_transaction_already_mined(web3, unlocked_account_dual_type)
finally:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, nice!

@pytest.mark.skip(reason="London TODO: crashes on [address_conversion_func1]")
@pytest.mark.xfail(
strict=False,
reason='Sometimes a TimeoutError is hit when waiting for the txn to be mined',
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this consistently fails on one error type, we should add the raises line back in so that we know it's flaky in the way we think it's flaky

@fselmo fselmo force-pushed the 1835-fix-tests-and-default-to-1559-fields branch from ca957fb to 1c178cd Compare June 30, 2021 20:16
- Fixed tests that were failing from the implementation of the new EIP-1559 fields (maxFeePerGas and maxPriorityFeePerGas). Updated most of our existing tests to use these new fields in order to encourage their use / be updated to the new standard.
- Updated documentation around unit and integration testing and how to contribute to our test suite.
@fselmo fselmo force-pushed the 1835-fix-tests-and-default-to-1559-fields branch from 1c178cd to 5e011f5 Compare June 30, 2021 20:50
@fselmo fselmo merged commit ade5f68 into ethereum:london Jun 30, 2021
@fselmo fselmo deleted the 1835-fix-tests-and-default-to-1559-fields branch June 30, 2021 21:59
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.

2 participants