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

Bump pytest to version 7 #2863

Merged
merged 1 commit into from
Apr 5, 2023
Merged

Bump pytest to version 7 #2863

merged 1 commit into from
Apr 5, 2023

Conversation

e3243eric
Copy link
Contributor

@e3243eric e3243eric commented Mar 7, 2023

What was wrong?

Related to Issue #2859. From CI warning: DeprecationWarning: You're using an outdated version of pytest. Newer releases of pytest-asyncio will not be compatible with this pytest version. Please update pytest to version 7 or later.

How was it fixed?

Bump pytest to version 7.

Todo:

Cute Animal Picture

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

@e3243eric
Copy link
Contributor Author

The plugin pytest-pythonpath is obsolete after pytest 7.0.0. Now pytest using the pythonpath configuration option.

@e3243eric
Copy link
Contributor Author

e3243eric commented Mar 7, 2023

The test_eth_get_balance_with_block_identifier failed because the block was not mined to block 1. This is because the test execution order has been changed by pytest. test_eth_get_balance_with_block_identifier was executed before some tests which mining blocks. (like test_eth_get_storage_at will mine a block because of the emitter_contract_deploy_txn_hash fixture)

Should I mine a new block by the block_with_txn fixture in test_eth_get_balance_with_block_identifier? Overwrite test_eth_get_balance_with_block_identifier in the TestEthereumTesterEthModule class? Or any other suggestions?

@fselmo
Copy link
Collaborator

fselmo commented Mar 7, 2023

Should I mine a new block by the block_with_txn fixture in test_eth_get_balance_with_block_identifier? Overwrite test_eth_get_balance_with_block_identifier in the TestEthereumTesterEthModule class? Or any other suggestions?

I'd rather address it in the TestEthereumTesterEthModule since the dynamics can be a bit different across providers. This would explicitly target the EthereumTesterProvider. There are different ways to do it I suppose but one of the most explicit ways may be to do something like:

    def test_eth_get_balance_with_block_identifier(self, w3: "Web3") -> None:
        # send some money to the coinbase account to ensure later balance > genesis
        w3.eth.send_transaction({"to": w3.eth.coinbase, "value": 1, "gas": 21000})

        super().test_eth_get_balance_with_block_identifier(w3)

thoughts?

@e3243eric
Copy link
Contributor Author

e3243eric commented Mar 8, 2023

Thanks for @fselmo! Your suggestion is great and clear. I push the same commit here. But I have a dumb question, why did you choose to send some value to the coinbase address? If any transaction is sent (same as block mined in EthereumTesterProvider), like sent to another address or sent with zero value, will the block reward to the coinbase address?

@fselmo
Copy link
Collaborator

fselmo commented Mar 8, 2023

will the block reward to the coinbase address?

Frankly, I don't remember off the top of my head if the eth-tester + py-evm combo handles the reward appropriately (it should) 😅. If it works to just mine a block then that's great too 🙂

@e3243eric
Copy link
Contributor Author

e3243eric commented Mar 8, 2023

I think it does. The test works until the execution order changes.

Since it is to test get_balance with a block number, I think sending a transaction to the coinbase address is a good idea, no matter where the block reward goes. I was curious about the block reward. 🙂
PR is ready for review.

@fselmo
Copy link
Collaborator

fselmo commented Mar 8, 2023

Yeah it should handle all of that appropriately but the value transfer is more explicit 👍🏼

@e3243eric
Copy link
Contributor Author

e3243eric commented Mar 25, 2023

I saw the PR #2887 dealing with pythonpath and test_eth_get_balance_with_block_identifier, so I only bumped Pytest here.

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.

Thanks for the contribution @e3243eric! I left a few comments that I can get to in the next few days if you aren't able/willing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a dev dependency change, let's call this filename newsfragments/2863.internal.rst

setup.py Outdated
@@ -28,10 +28,9 @@
"flaky>=3.7.0",
"hypothesis>=3.31.2",
"importlib-metadata<5.0;python_version<'3.8'",
"pytest>=6.2.5",
"pytest==7.2.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible, I'd rather bump pytest>=7.0.0 to allow a wider range. Does that work here?

@e3243eric e3243eric changed the title Bump pytest from 6.2.5 to 7.2.2 Bump pytest to version 7 Apr 4, 2023
@e3243eric
Copy link
Contributor Author

Thanks for all the reviews.

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.

Thanks @e3243eric! Looks good to me!

@kclowes kclowes merged commit ff698a9 into ethereum:master Apr 5, 2023
@e3243eric e3243eric deleted the bump-pytest branch April 6, 2023 01:37
@kclowes kclowes mentioned this pull request Apr 14, 2023
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.

3 participants