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

Remove Goerli and migrate network tests on sepolia #1328

Merged
merged 21 commits into from
Mar 29, 2024

Conversation

tkumor3
Copy link
Contributor

@tkumor3 tkumor3 commented Mar 21, 2024

Closes #

  • Remove support for Goerli
  • run network test on sepolia

Introduced changes

  • This PR contains breaking changes
  • Remove StarknetChainId GOERLI

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.74%. Comparing base (8d02f01) to head (71341b8).

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #1328      +/-   ##
===============================================
- Coverage        97.81%   97.74%   -0.07%     
===============================================
  Files               95       95              
  Lines             4752     4750       -2     
===============================================
- Hits              4648     4643       -5     
- Misses             104      107       +3     
Files Coverage Δ
starknet_py/contract.py 97.68% <ø> (ø)
starknet_py/net/account/account.py 99.59% <ø> (ø)
starknet_py/net/models/chains.py 93.33% <100.00%> (-0.42%) ⬇️
starknet_py/net/networks.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@tkumor3 tkumor3 changed the title migrate network tests on sepolia Remove Goerli and migrate network tests on sepolia Mar 22, 2024
@tkumor3 tkumor3 removed the blocked label Mar 26, 2024
@tkumor3 tkumor3 marked this pull request as ready for review March 26, 2024 10:42
docs/development.rst Outdated Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/client_test.py Outdated Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/client_test.py Outdated Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/client_test.py Outdated Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/client_test.py Outdated Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/trace_api_test.py Outdated Show resolved Hide resolved
@tkumor3 tkumor3 requested a review from ddoktorski March 27, 2024 08:00
.github/workflows/checks.yml Outdated Show resolved Hide resolved
.github/workflows/checks.yml Outdated Show resolved Hide resolved
starknet_py/hash/transaction_test.py Outdated Show resolved Hide resolved
starknet_py/net/networks.py Outdated Show resolved Hide resolved
starknet_py/tests/e2e/fixtures/accounts.py Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/client_test.py Outdated Show resolved Hide resolved
@THenry14
Copy link
Member

THenry14 commented Mar 27, 2024

Additional question - I did not check the logs, but I can see network tests are failing. what is the problem?

starknet_py/hash/transaction_test.py Outdated Show resolved Hide resolved
starknet_py/tests/e2e/fixtures/accounts.py Show resolved Hide resolved
starknet_py/tests/e2e/test-variables.env.template Outdated Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/trace_api_test.py Outdated Show resolved Hide resolved
starknet_py/tests/e2e/tests_on_networks/trace_api_test.py Outdated Show resolved Hide resolved
docs/development.rst Show resolved Hide resolved
docs/migration_guide.rst Show resolved Hide resolved
starknet_py/tests/e2e/fixtures/accounts.py Show resolved Hide resolved
starknet_py/tests/e2e/fixtures/devnet.py Outdated Show resolved Hide resolved
) -> Tuple[str, str]:
"""
Deploys an Account and adds fee tokens to its balance (only on devnet).
"""

network = run_devnet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced of this solution, any better idea?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this change tbh

Copy link
Contributor Author

@tkumor3 tkumor3 Mar 28, 2024

Choose a reason for hiding this comment

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

I removed fixture network which was used in a few places. Currently, we need only the devnet URL which can be obtained by calling run_devnet but IMO it's a little bit confusing:

@pytest.fixture(name="client", scope="package")
def create_full_node_client(run_devnet) -> FullNodeClient:
    return FullNodeClient(node_url=run_devnet + "/rpc")

the idea is to assign run_devnet to network and use

@pytest.fixture(name="client", scope="package")
def create_full_node_client(run_devnet) -> FullNodeClient:
    network = run_devnet
    return FullNodeClient(node_url=network + "/rpc")

But tbh I don't like this approach

Copy link
Member

Choose a reason for hiding this comment

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

what is run_devnets type? is this a function or a string? can we just pass the url, if this is all we need?

Copy link
Contributor Author

@tkumor3 tkumor3 Mar 28, 2024

Choose a reason for hiding this comment

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

It's Generator[str, None, None], so if you call run_devnet you will get a string

@pytest.fixture(scope="package")
def run_devnet() -> Generator[str, None, None]:
    """
    Runs devnet instance once per module and returns it's address.
    """
    devnet_port, proc = start_devnet()
    yield f"http://localhost:{devnet_port}"
    proc.kill()

before execute it we don't know devnet port

Copy link
Collaborator

Choose a reason for hiding this comment

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

For me it can be either run_devnet_and_get_url or simply devnet. In the second option, I would pass devnet directly without the assignment network = devnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

devnet sounds good!

Copy link
Member

Choose a reason for hiding this comment

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

let's do that then

@tkumor3 tkumor3 merged commit 3e8899d into development Mar 29, 2024
16 checks passed
@ddoktorski ddoktorski deleted the tomaszkumor/remove_goerli branch April 5, 2024 09:36
@ddoktorski ddoktorski mentioned this pull request Apr 5, 2024
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