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

test: a simpler way to disable mocktime in p2p_initial_headers_sync.py #6235

Closed
wants to merge 1 commit into from

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Aug 27, 2024

Issue being fixed or feature implemented

simplify mocktime disabling in p2p_initial_headers_sync.py

What was done?

How Has This Been Tested?

run p2p_initial_headers_sync.py

Breaking Changes

n/a

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@UdjinM6 UdjinM6 added this to the 21.2 milestone Aug 27, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK 175a46f

self._initialize_chain_clean()
else:
self._initialize_chain()
def setup_network(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using setup_network() seems as an ugly solution. Let's add this refactoring better.

If you think that's out-of-scope PR I will create a new PR

Copy link
Author

Choose a reason for hiding this comment

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

I like that! Pls submit a PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's here #6242

Copy link
Author

Choose a reason for hiding this comment

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

Let's see if CI is happy there and if it is I'm just going to close this one in fav of yours.

@UdjinM6 UdjinM6 marked this pull request as draft August 30, 2024 12:56
@UdjinM6 UdjinM6 removed this from the 21.2 milestone Aug 30, 2024
@UdjinM6
Copy link
Author

UdjinM6 commented Aug 30, 2024

closing in fav of #6242

@UdjinM6 UdjinM6 closed this Aug 30, 2024
PastaPastaPasta added a commit that referenced this pull request Sep 4, 2024
…rams()

ef4d74a test: remove dead code from `p2p_initial_headers_sync.py` to favor of disable mocktime (UdjinM6)
4d9837c refactor: add a new flag disable_mocktime to set_test_params() (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  To disable mocktime you should re-implement setup_nodes(). It seems as bug-friendly solution

  ## What was done?
  This PR introduce a new flag "disable_mocktime" which can be set in `set_test_params`.
  It seems more error prune and the code is shorter

  ## How Has This Been Tested?
  Run unit/functional tests including future changes from #6235

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK ef4d74a
  kwvg:
    utACK ef4d74a
  PastaPastaPasta:
    utACK ef4d74a

Tree-SHA512: c6be8002cae4d7824e150938957464c156931d0b6f7fc41c430a83d662865431f1b56cb11df73f56db54a140f36b0addd68b2917e25c5c941fae52e8d322bc25
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