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

backport: bitcoin/bitcoin#22229 test: consolidate to f-strings and related fixes #6357

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Oct 24, 2024

Issue being fixed or feature implemented

Just one backport bitcoin#22229 because it is a big size, even though there's nothing non-trivial in it.

Though, even it is called as part I, there has not been part II yet.

What was done?

Some preparation, code unifications to make bitcoin#22229 with less conflicts and finally backport of itselfl.

How Has This Been Tested?

Run unit & functional test

Breaking Changes

N/A

Checklist:

  • 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

Copy link

This pull request has conflicts, please rebase.

@knst
Copy link
Collaborator Author

knst commented Oct 24, 2024

This pull request has conflicts, please rebase.

That's a lie!

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK fc84368

test/functional/feature_notifications.py Outdated Show resolved Hide resolved
test/functional/feature_loadblock.py Outdated Show resolved Hide resolved
UdjinM6
UdjinM6 previously approved these changes Oct 24, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK a15e339

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK a15e339, good to merge once CI is happy

@UdjinM6 UdjinM6 dismissed their stale review October 25, 2024 07:03

linux64_sqlite-test keeps failing on feature_notifications.py

@knst
Copy link
Collaborator Author

knst commented Oct 25, 2024

CI fails for sqlite build https://gitlab.com/dashpay/dash/-/jobs/8184393307

@UdjinM6
Copy link

UdjinM6 commented Oct 25, 2024

reverting/dropping 1b6baa5 fixes it https://gitlab.com/UdjinM6/dash/-/pipelines/1511972465

@knst
Copy link
Collaborator Author

knst commented Oct 25, 2024

reverting/dropping 1b6baa5 fixes it https://gitlab.com/UdjinM6/dash/-/pipelines/1511972465

yes I am looking to the same commit, but not sure how exactly to fix it yet. I will try to find a fix for it and if it is not easy I will just drop it.

MarcoFalke and others added 2 commits October 25, 2024 21:24
68faa87 test: use f-strings in mining_*.py tests (fanquake)
c2a5d56 test: use f-strings in interface_*.py tests (fanquake)
86d9582 test: use f-strings in feature_proxy.py (fanquake)
31bdb33 test: use f-strings in feature_segwit.py (fanquake)
b166d54 test: use f-strings in feature_versionbits_warning.py (fanquake)
cf6d66b test: use f-strings in feature_settings.py (fanquake)
6651d77 test: use f-strings in feature_pruning.py (fanquake)
961f581 test: use f-strings in feature_notifications.py (fanquake)
1a546e6 test: use f-strings in feature_minchainwork.py (fanquake)
6679ece test: use f-strings in feature_logging.py (fanquake)
fb63393 test: use f-strings in feature_loadblock.py (fanquake)
e9ca8b2 test: use f-strings in feature_help.py (fanquake)
ff7e330 test: use f-strings in feature_filelock.py (fanquake)
d5a6adc test: use f-strings in feature_fee_estimation.py (fanquake)
a2de33c test: use f-strings in feature_dersig.py (fanquake)
a2502cc test: use f-strings in feature_dbcrash.py (fanquake)
3e2f84e test: use f-strings in feature_csv_activation.py (fanquake)
e2f1fd8 test: use f-strings in feature_config_args.py (fanquake)
36d33d3 test: use f-strings in feature_cltv.py (fanquake)
dca173c test: use f-strings in feature_blocksdir.py (fanquake)
5453e87 test: use f-strings in feature_backwards_compatibility.py (fanquake)
6f3d5ad test: use f-strings in feature_asmap.py (fanquake)

Pull request description:

  Rather than using 3 different ways to build/format strings (sometimes all in the same test, i.e [`feature_config_args.py`](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_config_args.py)), consolidate to using [f-strings (3.6+)](https://docs.python.org/3/reference/lexical_analysis.html#f-strings), which are generally more concise / readable, as well as more performant than existing methods.

  This deals with the `feature_*.py`, `interface_*.py` and `mining_*.py` tests.

  See also: [PEP 498](https://www.python.org/dev/peps/pep-0498/)

ACKs for top commit:
  mjdietzx:
    reACK 68faa87
  Zero-1729:
    crACK 68faa87

Tree-SHA512: d4e1a42e07d96d2c552387a46da1534223c4ce408703d7568ad2ef580797dd68d9695b8d19666b567af37f44de6e430e8be5db5d5404ba8fcecf9f5b026a6efb
@knst knst marked this pull request as ready for review October 25, 2024 14:40
@knst
Copy link
Collaborator Author

knst commented Oct 25, 2024

I dropped that problematic commit for feature_notifications.py; will resolve issue some other day

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK 7c6c93d

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK 7c6c93d

@PastaPastaPasta PastaPastaPasta merged commit 4b5e392 into dashpay:develop Oct 25, 2024
20 of 28 checks passed
@knst knst deleted the bp-v23-p1 branch October 26, 2024 04:00
@UdjinM6 UdjinM6 added this to the 22 milestone Oct 29, 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